Open joheriks opened 4 years ago
As we agreed: before making this as a validation rule we should check whether the automatic formatting in Eclipse still causes formatting changes after the code format is applied.
In fact I totally disagree to include a validation rule for formatting. It totally destroys any development: I know this from my experience that most of time is spent on formatting instead of real coding.
Formatting may not be a validation rule. Either we are able to reduce formatting changes via making sure that all formatters we are using in IDEs matches each other or this should work automatically without any involvement if formatting is absolutely necessary: validation should have a task to update the PR applying formatting automatically.
I may do only one thing:
I personally can't do both.
I'd like to point out again that the time is not only spent on writing the code, but also in review. And that time is also important to be spent efficiently and not having to care about looking on unrelated formatting changes.
I think the purpose is to get to a phase where nobody has to care about formatting related changes when either writing code or when doing review. To achieve this, I think what we need is
The validation should be fine for everyone as long as they use Eclipse or IDEA with the correct settings. It doesn't change anything. It only changes things for those people who have not setup their IDEs and try to push unformatted changes.
In that way I agree.
So if we are able to :
then I'm OK with this approach.
My point is : to be able to require formatting we have to define the rules exactly and make it easy to use. If we are not able to do this then we may not require this.
About the current situation :
since no-one else has this issue I assume that there is incompatibility between IDEA and Eclipse formatter and I don't see how the project reformatter may fix this: if the current formatting is not the right one then others would have had formatting differences as well.
I'm not sure, this would have to be tested, but I think the extra formatting changes coming from you are because there is somehow wrong line breaking rules applied. This has probably due to either: a) there is existing code in the project that has not been formatted properly b) there is a difference in how the current settings for line breaks are done for Eclipse formatter and IDEA (which uses eclipse code formatter plugin)
So I think there needs to be some investigation to compare the changes between both IDEs with the current setup to determine if there is something broken, or if we just have "unformatted code" that is now touched when using Eclipse with "format all lines" instead of "format only changed lines".
Well, that was my point: let's don't make validation formatting rule without understanding the issue and testing. The second item in acceptance criteria says: make automatic validation . Unconditionally. We should test and understand the current issue first and then based on that do it or not...
Just to clarify what I'm against of: I've been in a project where formatting rules where so strict that it wasn't even possible to have even one extra space (or one extra line). And there was no any good way to do autoformatting.
I spent 15 minutes on fixing a ticket (writing the real code) and several hours on fixing the formatting. So it was not about programming but about formatting. I left the project after 3 PRs . It was the worst experience in my life. I don't want to repeat this again ever.
The best way this can work is that everybody's IDE auto-formats everything correctly all the time. This would require using an external formatter and not different native IDE implementations which will never fully match.
Another way this could work is that everybody's IDE auto-formats everything almost correctly and there is a hook that auto-formats everything correctly all the time. The pre commit hook should be automatically activated on all developer's workspace, for old and new clones, or somehow integrated into GitHub. I would think the dev machine hook is more realistic.
The approach where you manually format code, this is actually what we already have in this project. We do not have anything enforcing it but if you try to figure out what you have actually changed after coding for a while, this is what you end up doing.
The goal here should be that with no work from anybody, PRs always only contain the changes done by the developer. This benefits everybody.
The goal here should be that with no work from anybody, PRs always only contain the changes done by the developer.
That's what I want: don't spend any time on this. It should just works. 👍
In addition to what Artur concluded about using a pre-commit hook that formats through mvn formatter:format
so that it's the same regardless of IDE (including small variations between different versions of the same IDE), we should also have mvn formatter:validate
as part of the validation build to ensure that everyone is actually using the pre-commit hook (or manually running mvn formatter:format
)
There are Maven goals
formatter:format
andformatter:validate
in place using the Flow conventions (https://github.com/vaadin/flow/blob/master/eclipse/VaadinJavaConventions.xml), but the code is not up to date with these. This causes unwanted formatting changes in PRs when the IDE to formats a source files (alternatively the developer has to take care to format to only modified code).To do: