Should we favor tabs or spaces in the checkstyle plugin?
Closed this issue · 6 comments
Is your feature request related to a problem? Please describe.
consistent formatting would make PRs faster
Describe the solution you'd like
I'd be willing to add the checkstyle rule for whitespace i just need to know which. personally i prefer spaces since they are consistent between editors.
I personally favor spaces as well since you can adjust your IDE to replace a TAB with spaces
The reason there isn't a rule here is - back in the day (before I joined) - the vote of active code people split 4 for spaces and 4 for tabs.... not sure that matters any more but just for background. I've never bothered to push it one way or the other because I've seen so much inconsistent code and frankly, the principle of avoiding unnecessary whitespace changes is important enough that it really doesn't bother me that things are inconsistent,.. I just don't change it.
I have a few concerns here; not related to a specific checkstyle patch, but the resulting side effects
First, we should be limiting functional and non-functional changes happening at the same time. If an entire file shows as changed but there was a one line functional change, that is very hard to review. There are a number of places in the PCGen code base that are ... quirky, and naive analysis can cause people to propose changes that will cause functional or performance issues (e.g. things are loaded with reflection, variables are a stateful cache across multiple calls, etc.). In general, even when I'm idle to producing code, I am reviewing PRs to make sure they aren't running into our quirks.
Second, if any bulk changes will happen (which may be an idea to avoid the issue above), then let's keep them together as a series of merged commits ... without any functional code patches intervening (data are fine). This is because I have >100 local branches with unfinished changes. The merge / rebase across such bulk changes is very time consuming, so isolating it to one set of commits I can approach and then jump in one step (knowing it's only non-functional) is very helpful. Even though I haven't committed lately, I also don't think this is a big ask.
p.s. I won't hold this up from being spaces, but I do prefer tabs since I navigate with arrows and that is much faster with tabs
you're right, maintaining whitespace is fine.
at this point there are so many different IDE's that handle whitespace for you, and for this project, you have to turn off some of that because of the inconsistent spacing. if we standardized it, we'd have fewer whitespace changes in functional changes.
we are also using checkstyle, which should ensure that passing code is ready to merge, but in our case there's one more thing that can be wrong.
To be clear: just because I'm trained to ignore it doesn't make it the best answer :)
IMHO you are also correct that longer term it will help with check-in clarity if we have a standard. So this really speaks to doing a bulk update. There are code formatters that can be run, and we've done one before - James did one back in the 5.16 era I think? Right about the time we adopted Checkstyle. Fixed all the brackets/curly braces, CR/LF, and such (though I think there is one of those checkstyle settings that isn't quite correct and allows a bracket at the end of a line. (in elseif?).
The advantage of doing such a bulk update is we also could make line wrapping consistent. Some of our stuff is set to 80, some 90, some higher. Some of it's really ugly at 80, so we probably want a higher number, but not sure we have an agreed upon standard there. I think Travis doesn't complain until like 120...
Are there any other opinions or should i open a PR with the reformat and the rule as soon as i'm not horrendously busy?
I think i've given this enough time, i'll open the PR this weekend.