droid icon indicating copy to clipboard operation
droid copied to clipboard

Overly pedantic checkstyle formatting rules.

Open nishihatapalmer opened this issue 3 years ago • 8 comments

Some of the checkstyle rules defined for DROID are overly pedantic. They just put annoying obstacles in the way of contributing. I usually end up having to spend several hours reformatting import orders, putting spaces after certain operators, etc, moving boolean operators on to new lines...

Having a good consistent style is good to maintain clarity, and things like cyclomatic complexity are good quality indicators. I can honestly say that basic code formatting isn't something that prevents me from understanding code in general.

Any chance we can review and relax some of these rules, particularly ones which insist on precise formatting of whitespace and import orders?

nishihatapalmer avatar Feb 07 '21 15:02 nishihatapalmer

I agree @nishihatapalmer There are some rules that are pedantic. I think we can relax some. I'll add the specific rules to the same issue when we encounter. Any thoughts?

sparkhi avatar Feb 08 '21 09:02 sparkhi

Examples of rules that may be considered pedantic:

  1. Copyright header does not allow for the year to be different within different source code files

sparkhi avatar Feb 08 '21 09:02 sparkhi

Copyright year should be consistent across all files and should be the inception year of the project. I think this was previously discussed - I don't remember if it was an issue or done by email with @Dclipsham

adamretter avatar Feb 08 '21 10:02 adamretter

Copyright year should be consistent across all files and should be the inception year of the project. I think this was previously discussed - I don't remember if it was an issue or done by email with @Dclipsham

I think it was an issue I raised. If that's the policy then fine, we can close that issue (I think it's still open). I don't mind just putting project inception date in the header, it's actually less work than keeping the dates current.

nishihatapalmer avatar Feb 08 '21 10:02 nishihatapalmer

@nishihatapalmer You should not have to maintain it manually.

You can use mvn license:check and mvn license:format

adamretter avatar Feb 08 '21 11:02 adamretter

copyright year was mentioned here: https://github.com/digital-preservation/droid/pull/180

Dclipsham avatar Feb 08 '21 11:02 Dclipsham

Next 2 that I encountered just now

  • Ordering of imports
  • Ordering of the instance variables in relation to static variables

sparkhi avatar Feb 08 '21 14:02 sparkhi

These rules feel the most pedantic and unhelpful to me - essentially those relating to code layout.

  1. Ordering rules.
  2. Whitespace rules.
  3. Formatting rules for common code constructs (e.g. can't put boolean operator at the end of a line, has to be at the start).

My take is that unless a positive case can be made for particular rules in these categories, they should be disabled. There may be some we want to keep. Essentially - take a whitelist approach to code layout rules - you have to positively turn it on and justify it, rather than having all of them and picking them off one-by-one.

In general, variations in style doesn't seem to hurt my understanding of code. Everyone has slightly different approaches, and I'm OK with that. Sometimes I see a different approach I prefer!

nishihatapalmer avatar Feb 12 '21 12:02 nishihatapalmer