Add Gradle tasks and Git hooks for IntelliJ-based code formatting (#15535)#15555
Add Gradle tasks and Git hooks for IntelliJ-based code formatting (#15535)#15555sanjana2505006 wants to merge 1 commit intoapache:7.0.xfrom
Conversation
56037b6 to
e886bca
Compare
|
@sanjana2505006 we explicitly decided to not auto-install the hook so it would be optional if people wanted it. |
| .map(Long::parseLong) | ||
| .map(Instant::ofEpochSecond) | ||
| .orElseGet(Instant::now) | ||
| .filter { s -> !s.isEmpty() } |
There was a problem hiding this comment.
Please revert these changes.
|
To revert the changes in build.gradle, remove the added plugin block and revert the lambda expressions in the ext block back to method references. build.gradle |
| private static void doNotApplyStylingToTests(Project project) { | ||
| project.tasks.named('checkstyleTest') { | ||
| it.enabled = false // Do not check test sources at this time | ||
| if (project.tasks.names.contains('checkstyleTest')) { |
There was a problem hiding this comment.
Can you help me understand why this change was necessary?
| } | ||
| } | ||
|
|
||
| private static void registerFormattingTasks(Project project) { |
There was a problem hiding this comment.
The code style work is planned to move to it's own repository (eventually). I'm ok with putting the formatting logic adjacent to this plugin, but I don't think it should be in this plugin since it's effectively a root only plugin - the root project won't ever have sources. Let's move this to it's own format plugin?
| formatExec = new File(ideaHome, "bin/$executable") | ||
| } else { | ||
| // Try common paths on macOS | ||
| if (Os.isFamily(Os.FAMILY_MAC)) { |
There was a problem hiding this comment.
We should just make it a requirement that idea is on the path and reference the version on the path. IntelliJ has an option to install the command line tools (https://www.jetbrains.com/help/idea/working-with-the-ide-features-from-command-line.html) so let's just reference the help article and not trying to be smart about locating it (anyone using JetBrains toolbox won't be installing it to /Applications or to a specific location)
| } | ||
|
|
||
| if (formatExec == null || !formatExec.exists()) { | ||
| project.logger.error("IntelliJ formatter executable not found.") |
There was a problem hiding this comment.
It's deprecated to reference project from inside of a task. logger should be available already.
| throw new RuntimeException("IntelliJ formatter executable not found.") | ||
| } | ||
|
|
||
| def filesToFormat = project.findProperty('formatFiles') |
There was a problem hiding this comment.
I hadn't considered the useful of having a format task added to the subprojects. I still think we should split the format tasks out from the code style though - otherwise you're trying to apply styling to the root project which we don't want to do.
| throw new RuntimeException("IntelliJ code style settings not found at ${settingsFile.absolutePath}") | ||
| } | ||
|
|
||
| project.exec { ExecSpec exec -> |
There was a problem hiding this comment.
Take a look at the publishAllToMavenLocal task for how to do in this a modern Gradle way without referencing project .
Added a
formatCodeGradle task that uses the IntelliJ CLI formatter to keep our code style consistent. I also added aninstallGitHookstask that sets up a git pre-commit hook to automatically format staged files. This should help avoid formatting issues during review. Fixes #15535.