-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Formatting the entire code base #1476
base: trunk
Are you sure you want to change the base?
Conversation
No. This needs proper discussion in the Jackrabbit PMC and likely a vote because it'll be controversial. |
I put this as a draft just to test it and see how the PR would look like :) |
I can see that deciding on which formatter we should use could warrant a bit of discussion. But I find it strange that introducing one would be controversial. On the contrary, the fact that we don't have one is concerning. Especially considering how easy it is to introduce and that it won't clutter the blame view. |
@steffenvan, it messes up diffs. |
@mbaedke how so? |
@steffenvan, If you - for example during debugging - compare between revisions to see what has changed and git will always tell you that everything did, it's very annoying. |
That's why we can put the formatting commits inside .git-blame-ignore-revs |
but that doesn't help in all cases, for instance:
|
Also we are talking git diff, not git blame, right? I guess you can use git blame in an intermediate step to produce a list of files for the diff, but really??? |
May I ask what's so critical about that? |
to me that also sounds like a one time thing. If both revisions have the same formatting, this should not be a problem? |
So do you propose to reformat the current maintenance branch and historic branches as well? What if we want to backport a commit to the maintenance branch? |
Then we backport the code with the new formatting applied? The formatting doesn't change the functionality of the code. |
Again, if you want to detect what has changed at all and you compare with an old revision, it's very annoying. |
Do you expect cherry-pick to run smoothly in this situation? |
Depending on the amount of changes, it's going to be pretty simple to fix the conflicts yeah. Just keep the formatting in one commit and cherry picking in another. Besides, how often do we need to backport? And how often are we maintaining, refactoring and adding new features in Oak? And more importantly, which one are we doing the most? |
Working on a codebase with inconsistent formatting is also not without friction. |
We regularly backport things, and the amount of work when cherry-picking is impossible is dramatically higher. |
Question: |
Maybe it's better to move the discussion to the mailing list: https://lists.apache.org/thread/0519b0m34m8853zxncmt4cc6omo155k4 |
Because 1.22 is our maintenance branch. Anyway, I don't think we're having a productive discussion here. |
That makes sense. My Prediction: you will face a lot of resistance :) |
I already am ;) Doesn't change the fact that it brings huge advantages. There is a reason most organizations have a very strict guideline. |
Final remark here: It's not just Whitespace/EOL if the Javadoc gets reformatted. |
I do. I do a lot! That's how I find out why the code is like this. This is called "Chesterton's Fence." and it's quite an important part of what code maintenance means. |
Well let's discuss here, and then if we use the mailing list we need a link there. |
@thomasmueller, what I wanted to point out is that even if the blame view looks fine, there is much more to it than that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes thousands of files. This is a blocker for me.
Reformatted all the code with https://google.github.io/styleguide/javaguide.html without polluting the history.
See: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view