ignore Eclipse files + incremental 4-space indent Java formatter #254
Conversation
pom.xml
Outdated
| - Apply Spotless to specific files: https://github.com/diffplug/spotless/tree/main/plugin-maven#can-i-apply-spotless-to-specific-files | ||
| --> | ||
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> |
There was a problem hiding this comment.
Indentation in this file: Please stay consistent, and don't use tabs. 4 spaces??
There was a problem hiding this comment.
Done. Fixed the inconsistent indentation.
That being said... since the rest of the file is using tabs, I made one commit to make the indentation consistent with the tab-based indents. The subsequent commit converts the entire file from tabs to spaces.
It seems that multiple *.xml files in the repo use tabs, not spaces. Interested in adding a 4-space indent rule for certain files to the formatter / style checker?
There was a problem hiding this comment.
It seems that multiple *.xml files in the repo use tabs, not spaces. Interested in adding a 4-space indent rule for certain files to the formatter / style checker?
Yes, I think so.
@josh-hadley it looks like the tab checker does not look at XML files?
We probably can't enforce much about .txt files. Or maybe none of them even use indentation.
There was a problem hiding this comment.
Right, tab checker only looks at files ending with .java. Easy enough to add XML or others if desired.
There was a problem hiding this comment.
our xml files [pom.xml, build.xml, all cldr data files] have traditionally used tabs. Is there a reason to change?
There was a problem hiding this comment.
Looks like icu4j's build.xml uses spaces, though
There was a problem hiding this comment.
Tabs are generally trouble because there is not a standard advance width for them. Many tools default to tab stops every 8 columns, but some people set some of their tools to 4, and when files use a mix of tabs and spaces (which often happens) the indentation is all messed up in one editor / code review tool or another. Or indentation with tabs and vertically aligned comments at the ends of lines. That's why we try to avoid tabs.
| # for the git remote name for unicode-org/unicodetools? | ||
| - name: Fetch git branches for unicode-org/unicodetools | ||
| run: | | ||
| git fetch origin |
There was a problem hiding this comment.
why is this a separate step? is it because actions/checkout has fetch-depth: 1 by default?
There was a problem hiding this comment.
or is this a different fetch than the PR's fetch?
There was a problem hiding this comment.
Done. Thanks for pointing out the fetch-depth config for actions/checkout, I'm using that instead. The reason for having the git fetch in the first place is because of the Spotless configuration for the "ratchet" feature that only checks formatting diffs to modified files in the PR (and it needs the reference point origin/main to diff and grok what's in the PR).... however, we are leaning towards having a single big bang change globally for formatting, which will remove noise from future PRs by allowing those future PRs to only spotlight the changed lines of interest. That makes sense, and would obviate the need for the ratchet feature.
…ssure that chnages are committed
|
I've updated this PR to include both: 1) adding the Eclipse project files to Note: One detail that I encountered is that one of the CI jobs to auto run tools for smoke test purposes runs a tool (GenerateEnums) that generates some source code, and then does a diff, according to documented instructions, to ensure changes are committed. So in that CI job, the formatter needs to be applied after GenerateEnums regenerates source code but before the The main files to look at should be the root |
Based on previous discussion about #217, if we start ignoring Eclipse project settings dot files/folders in the codebase, we may also lose consistent source formatting (for those who use Eclipse).
Before progressing, we wanted to see if we could also handle source code style & formatting functionality in an IDE-agnostic way (while still having Eclipse support). This is similar to what we have done using Maven for build functionality.
This PR adds a couple of commits on top of #217 to add a Java source formatter that supports 4-space indents, and it only applies itself to files in a PR branch that have been changed (relative to the latest state of the codebase in the upstream repo).
This PR is a cleaned up version of echeran#5, whose checks pass. Also, a negative test PR echeran#6 adds a commit to its branch which touches a single file to create 2-space indents, and the checks fail.