Triplea: Checkstyle rule reconciliation - variable naming

[DanVanAtta]

It's kinda painful to find checkstyle violations after committing code. Largely this is because we have so many, finding new ones is a needle in haystack.

Beyond the practical impact, it's not good we have violations that we just cannot fix. Grant it, I very much like that we have went to some great lengths to ensure we do not make violations worse. This I think has set a strong working precedent. At the same time, I don't think the incentive of doing a non-compatible release just so we can fix naming violations is a good one to have. So I think there is a larger picture impact here, that we are more focused on the violations potentially rather than doing the right thing and adding project value.

Proposal

Given that we:

• have set a strong standard on variable names (done)
• are actively working towards removing 'm_' variables (done)

It is proposed:

• update checkstyle to allow 'm_' variables names (with the understanding this is something we wish to remove)

Expected benefits:

• drastic reduction in checkstyle violations, this has a number of follow-up benefits:
  
  
  
  • easier to find new ones
  
  
  • i'm guessing given this group, if we can get the violation count under 300 by removing the naming violations and have things we can actually fix, that we'll get it down to zero in pretty short order :grin:
  
  
• correction of incentives. Channeling @ simon33-2 and @Deltium here : )
  
  
  
  • doing an incompatible release just so we can fix variable naming is IMHO a bad incentive to even have.
  
  

[ssoloff]

If we're truly moving forward with breaking compatibility, then we might as well just fix the violations. Otherwise, we already have precedent for suppressing such violations with a comment indicating they should be fixed upon the next incompatible release. For example:

@SuppressWarnings("checkstyle:MemberName") // rename upon next incompatible release
private Attachable m_attachedTo;

IMO, that would be preferable to altering the Checkstyle configuration and possibly forgetting to revert it in the future.

[ron-murhammer]

@DanVanAtta We aren't doing an incompatible release just to fix variable names. Its just something that we should do while we are doing an incompatible release. Given that I really would rather fix than ignore them.

[DanVanAtta]

I know we're not doing it just to fix variable names. The incentive is a bad one.

Can we still fix all of these problems blindly? Would that break map compatibility?

My goal is that we do not ignore them, but also to reduce the count so we don't make development miserable:

• really hard to find new violations
• travis output is an avalance of checkstyle violations, browser window/travis locks up even, hard to find important info in the noise.

I'm happy with us directly fixing these problems, after-all that is the end-goal. Can we actually fix everything? If not, I suppose the remaining few examples that are needed for map compatibility (if any) would just get annotations.

I would suppose that is the plan, do I understand that correctly?

[ron-murhammer]

@DanVanAtta Yep, that was my thought and I think we can fix most or all of them as the main limitation for most of them is save game compatibility.

[DanVanAtta]

@ssoloff @ron-murhammer new idea: add suppression for all existing checkstyle violations. Benefits:

• removes the noise from travis and local build. This makes other failures stand out reducing time to find them.
• simplify the build infrastructure; while the automatic PR is a nice solution, we would not need any of that infrastructure anymore with violations at zero.

Thoughts?

[ssoloff]

:-1: As I said in #4143, I plan on fixing the remaining Checkstyle naming violations at the end of October. If, for some reason, it's no longer possible at that time to break compatibility, I will resubmit #4143.

[ssoloff]

BTW, are you using an IDE plugin for Checkstyle? It makes it a heck of a lot easier to see new violations flagged as you introduce them in real-time rather than looking through the Gradle build output.

[DanVanAtta]

@ssoloff emphasis on _all_ in" add suppression for all existing checkstyle violation", not just naming. Note the benefit we could simplify the build & CI by suppressing it down to zero.

The pain comes for committed code that is already out for PR. The easiest way to find violation is by adding all modified files and then checking files in the changeset. If stuff is already committed then this involves creating a parallel branch and then undoing commits so those files can be re-added so I can see where the violations are. The violations leak in as I have other checks turned on beyond our style guidelines. Even when working in eclipse, I like to dial the static analysis checks to max and then dial them down as it makes sense. There are many interesting violations that are not turned on by default IMO.

The net effect is that there can be many violations/errors in a file, so them creeping in is not that difficult. I think tweaking the default format would perhaps help too and make me less reliant on the auto-formatter, but is a different topic.

flagged as you introduce them in real-time rather than looking through the Gradle build output.

When looking at gradle built output, it's only on travis. The question there is, 'why failure?', and not clear that it's checkstyle or test. If test, the checkstyle error volume can make it hard to efficiently find those test failures. If a checkstyle violation, then it's a matter of doing the paralle branch dance to find where the violations are. Trying to diff out of that many violations is not feasible. I'm not sure how the gradle output for that many checkstyle violations is really useful at all.. IMO all the more reason to suppress them instead of ignore (even if we have a fix for a good 60% of them coming soon)

[ssoloff]

emphasis on all in" add suppression for all existing checkstyle violation", not just naming.

After fixing the naming violations, there will only be ~200 missing Javadoc violations. Piece of cake to knock those out, especially if everybody pitches in. :smile:

---

Here's some thoughts on ways I've found to quickly track down the root cause of a build failure:

The violations leak in as I have other checks turned on beyond our style guidelines. Even when working in eclipse, I like to dial the static analysis checks to max and then dial them down as it makes sense.

In Eclipse, Checkstyle violations are grouped separately in the Problems view so they don't get drowned out by other static analysis rules. In addition, the Checkstyle plugin provides a dedicated view that groups violations by rule:

Drilling down into a rule, you can usually narrow down a new violation quickly by file because you know which files you've changed as part of a PR:

The question there is, 'why failure?', and not clear that it's checkstyle or test.

Even in parallel mode, the end of the log should clearly state what task failed the build.

If test, the checkstyle error volume can make it hard to efficiently find those test failures

Once you know the test task that failed (e.g. :game-core:test), searching for that task name in the log will pretty quickly bring you to its output, even if it's sandwiched between hundreds of lines of Checkstyle output.

If a checkstyle violation, then it's a matter of doing the paralle branch dance to find where the violations are.

This is where leaning on the tools provided by the Checkstyle IDE plugin shines, as described above.

If you're stuck without an IDE plugin, the easiest way I found to quickly narrow down new violations is to diff Checkstyle XML reports (located in _game-core/build/reports/checkstyle/main.xml_) between the PR branch and the base branch. Again, because you know what files were changed in a PR, it's usually pretty easy to narrow down the culprit. No need to fork the PR branch.

[DanVanAtta]

After fixing the naming violations, there will only be ~200 missing Javadoc violations. Piece of cake to knock those out, especially if everybody pitches in

Some thoughts in response:

• There is no need to wait to start fixing them
• Not all will be worth javadoc'ing, I'd like to get us out of the culture of ignoring warnings/violations

In Eclipse, Checkstyle violations are grouped separately in the Problems view so they don't get drowned out by other static analysis rules. In addition, the Checkstyle plugin provides a dedicated view that groups violations by rule:

You get similar grouping in IDEA:

The noise comes in at the file level, a typical file is pretty littered with problems:

(notice the yellow ticks on the right hand side, those are all problems, whether checkstyle or otherwise).

So there is 0 problem with distinguishing between errors and checkstyle violations, or finding them, but it's still a PITA when 10~15 core TripleA files have been touched when each have 20 violations a piece. Running the scan on the full project also makes it a PITA to even find those files out of the full set, again we're doing a lot of back and forth work to know which files and where.

Overall it seems like we're just shooting ourselves in the foot. This level of effort seems counterproductive when the goal of checkstyle is to save time in review when known issues are introduced. The value of going back to fixing these naming variables and javadocs is pretty low, IMO the thing to do is to suppress all warnings, clean up our infrastructure, and then remove the warnings as we please where appropriate in the project. I am reminded of the first mass reformats and I was asked to make the formatting very nice verywhere. That was a month before major deletes came along that removed 1/3 of those files. I suspect we could see the same with some of these efforts, so I advocate we go ahead and jump to our end-goal of zero violations by suppressing the existing ones, I think we should have done that to start TBH (not faulting necessarily faulting our solution, but in hindsight it would have been better just to mass suppress and fix opportunistically)

The question there is, 'why failure?', and not clear that it's checkstyle or test.

Even in parallel mode, the end of the log should clearly state what task failed the build.

It does, but if it's a test, and you don't know which one, it is a PITA to find the few lines that tell you, or the additional lines of the test output. This is a real problem. I'm not sure what we gain by keeping the violations around so we can stare at them reminded all the time of things that are low priority fixes. I'm eventually getting very concerned about the priority inversion problem this creates. Again, the goal is to avoid time in code reviews nitting about new style violations

This is where leaning on the tools provided by the Checkstyle IDE plugin shines, as described above

Yes, but again, introduce 1 or 2 violations across 20 files where each file has many existing violations in them, it's not fun, even with tooling.

Again, the easiest way to find this is to get all changes to be grouped in the modified file set, so that does mean popping commits so that the files can be staged.

Again, because you know what files were changed in a PR

Sure, and not much of a problem when it's just one or two, it becomes a real time waste though when talking about 5 or more, particularly when it's in existings code that has many violations already. Hence the 'supress-all', and let's stop the ineffficiency.

[DanVanAtta]

@ssoloff not sure if this context is useful, the IDE is not the problem for me. I used eclipse starting around 2003~2004 up until 3 years ago when professionally had to switch running into IDEA only shops. Since then I have not met but one or two professionals still using eclipse, the gap between the two products IMO has really grown over the last 7 years from marginal to real advantages in IDEA where I no longer would want to switch back.

[DanVanAtta]

Suppress all violations, we gain the following benefits:

• simplify build
• remove noise
• new violations are even more easily found
• (and we still satisfy the original goal/purpose behind having and introducing checkstyle in the first place, namely to prevent "m_" and other legacy naming from spreading to new code)
• easy to find (with grep or IDE 'find-all') the violations. This IMO is perfect as we can find and notice them when we want to; otherwise the suppression let's us focus on new code.
• Zero violations is better enforcement, if you introduce a violation, you can find any existing violation and fix that for a net gain of zero. This violates the spirit of why we have checkstyle.

I'm on board with fixing the violations, but I'm concerned about priority inversion and a long tail of violations lingering around. Given that benefit list, and the one-time cost of one person to add the supressions, I think this is an easy choice. @ssoloff @ron-murhammer final thoughts? Let's make a decision here:
A) continue ignoring violations reported, enforce only a net gain of zero, continue executing on plan to fix many ASAP and then the rest in the medium term
B) zero out the violations with suppression (this ensure no new violations are added), simplify build and remove noise, and fix the variable naming ASAP and then fix the rest when the time is right

[ssoloff]

(C):

1. Fix all Javadoc violations.
2. Fix all naming violations that do not break lobby and XML compatibility.
3. Suppress remaining naming violations that would break lobby or XML compatibility.
4. Zero out thresholds and remove threshold update script.

[DanVanAtta]

Suppress remaining naming violations that would break lobby or XML compatibility.

On this step, while we're there, if we could annotate each of these places so we know later that there is a dependency, IMO that would be quite huge :champagne:

[ssoloff]

As of 1.10.0.0.12971, all Checkstyle naming violations have been fixed, or, where compatibility is required, suppressed.

There remain 215 missing Javadoc violations. I will start hitting those in small batches. If anyone else wants to pitch in, especially for areas you may have worked in recently, it would be appreciated. :smile:

[ssoloff]

As of 1.10.0.0.13158, all Checkstyle violations have been fixed, or where compatibility is required, suppressed. I will remove the automatic Checkstyle threshold update script in the Travis build shortly.