Spotless/tidy now applies to gradle build files #4009
Conversation
github-actions
bot
added
documentation
malliaridis
left a comment
malliaridis
left a comment
There was a problem hiding this comment.
Adding lint checks and linting on these files was on my bucket list. It's nice to see these files being formatted with consistent indentations now. 😄
I've added a few comments as thoughts while going over some formatting cases. But overall it looks good to me as is already. :)
| opts << [ | ||
| '-overview', | ||
| project.file("${srcDirs[0]}/overview.html") | ||
| ] | ||
| opts << [ | ||
| '-sourcepath', | ||
| srcDirs.join(File.pathSeparator) | ||
| ] | ||
| opts << [ | ||
| '-subpackages', | ||
| 'org.apache.solr' | ||
| ] |
There was a problem hiding this comment.
I'm curious what rule was applied here that caused the line breaks. It doesn't look like max line length to me, as we are under 50 characters in some of these lines. I would assume "chop down arrays when more than 1" would be the rule here, but it did not format the values below.
There was a problem hiding this comment.
Makes no sense to me -- which is sad because one would expect consistency to be a virtue of a formatter
| text = text.replaceAll( | ||
| /package (.+)/, | ||
| '''package $1 | ||
| '''package $1 | ||
| import javax.annotation.processing.Generated; | ||
| import org.apache.lucene.queryparser.charstream.CharStream;'''.trim()) | ||
| text = text.replace( | ||
| "/** Token Manager. */", | ||
| '''/** Token Manager. */ | ||
| "/** Token Manager. */", | ||
| '''/** Token Manager. */ | ||
| @Generated("JavaCC")''' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This part does not look very consistent, one case has the closing parentheses at the end, the other in a new line.
There was a problem hiding this comment.
True. I manually adjusted the closing parenthesis and the formatter didn't fight me on it. So consider this one spot fixed.
| public void close() { | ||
| // no-op. we close this stream manually. | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe there is some kind of inconsistency here with the spaces. We have 6 spaces indentation for some reason here, I believe a result of two different rules with different indentation. Is this expected?
Plus, I believe the closing curly bracket should be on the same indentation as the line that contains the opening bracket?
There was a problem hiding this comment.
RE indentation: wow... ugh...
RE braces: I don't see the issue
| withSignedArtifacts = { | ||
|
|
||
| -> |
There was a problem hiding this comment.
Interesting to so that this is a valid format. 🤔
There was a problem hiding this comment.
that newline is weird
|
I really appreciate the detailed inspection @malliaridis ! I am too dissatisfied with inexplicable inconsistencies to accept this as-is. Furthermore, this formatter is very slow. It takes a good deal longer than java and there's way more java. The Lucene project made it disable-able because of how slow it is. I think instead I'm simply going to have IntelliJ do a one-off reformat of all the gradle files, according to the rules we have in |
|
I stashed aside a simpler spotless config to do very basic things line removing trailing whitespace, ending newline, no tabs. I'll defer that for later. |
3 participants
Review this commit by commit, not all at once. The "tidy" one is the big one; maybe skim it a little. I'd prefer to land 3 commits :
.git-blame-ignore-revsThe tidying -- prefer to land this on not only main, branch_10x, and branch_10_0.
To reduce risk, branch_10_0 doesn't need spotless.gradle.
BTW Lucene has been doing this for awhile now. I took the same configuration choices. Didn't take its build code because they shifted to Java based Gradle plugins.