Skip to content

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented Apr 19, 2020

No description provided.

@lgoldstein
Copy link
Contributor

I am not sure how this change is supposed to work

  • Does one commit changes only after the build ? I find this slightly counter-intuitive as it means that what I see in the IDE is not necessarily what is committed.
  • Personally, I am not so comfortable with the result - I find it difficult to read due to the indentations
    *I think we are losing some features that checkstyle provides - it's not only about indentation

@gnodet
Copy link
Contributor Author

gnodet commented Apr 19, 2020

I am not sure how this change is supposed to work

  • Does one commit changes only after the build ? I find this slightly counter-intuitive as it means that what I see in the IDE is not necessarily what is committed.

The formatting happens during the maven build in the process-sources phase, so before the compilation / tests. What you see in the IDE will always be what you commit, as the formatting is done done during the commit phase, but during the maven build. Checkstyle runs during the maven build, so you can also commit things that do fail in maven, even if the IDE does not complain. The difference is that with this change, you won't get a failure, but it will reformat the source to adhere to the rules instead of simply failing with sometimes very cryptic or unintuitive formatting problems.

  • Personally, I am not so comfortable with the result - I find it difficult to read due to the indentations

The indentation is mostly the same afaik. If you refer to the 8 spaces continuation indentation instead of the 4 spaces, it can be changed. All formatting rules can be changed at will using the sshd-eclipse-formatter-config.xml config file in the root.

*I think we are losing some features that checkstyle provides - it's not only about indentation

Checkstyle is about the style. We have PMD for semantics.

Honestly, i find checkstyle very tedious to work with.

@lgoldstein
Copy link
Contributor

All formatting rules can be changed at will using the sshd-eclipse-formatter-config.xml config file in the root.

What about Intellij ? Is it affected in any way (since the file is called sshd-eclipse-formatter-config.xm)

Checkstyle is about the style. We have PMD for semantics.

I think that's inaccurate - the way I configured the tools is to complement each other - they each have some feature that the other does not. Furthermore, 'style' is not limited to indentation. IMO, there are many useful extra checks that checkstyle applies (see my reply on the dev mailing list)

Honestly, i find checkstyle very tedious to work with.

Matter of preference - I find it very useful :-) - I guess I will adjust to not having it...

@gnodet
Copy link
Contributor Author

gnodet commented Apr 19, 2020

All formatting rules can be changed at will using the sshd-eclipse-formatter-config.xml config file in the root.

What about Intellij ? Is it affected in any way (since the file is called sshd-eclipse-formatter-config.xm)

The eclipse formatter is used during the build, but this has nothing to do with the eclipse IDE.

Checkstyle is about the style. We have PMD for semantics.

I think that's inaccurate - the way I configured the tools is to complement each other - they each have some feature that the other does not. Furthermore, 'style' is not limited to indentation. IMO, there are many useful extra checks that checkstyle applies (see my reply on the dev mailing list)

Not sure what you refer to exactly here. I think the formatting takes into accounts all style related rules, spaces, braces, indentation, etc... You may have seen the changes in indentation, but that only means all other are the same.

The effect is that if you ommit a space or add spaces that would have been rejected by checkstyle, with this commit, they would simply be added / removed to follow the rules. Same for long lines, braces, etc...

Honestly, i find checkstyle very tedious to work with.

Matter of preference - I find it very useful :-) - I guess I will adjust to not having it...

I think you'll still have all the rules enforced. The only difference is that it will be done for you, instead of rejecting the build and force the developper to go back to the source code and fix the problem.

@lgoldstein
Copy link
Contributor

Not sure what you refer to exactly here. I think the formatting takes into accounts all style related rules, spaces, braces, indentation, etc... You may have seen the changes in indentation, but that only means all other are the same.

Checkstyle also has metrics - such as max. lines per method, max. lines per file, max. parameters per method, enforce @Override annotation, rules for variables naming, visibility fields orderings, etc.,
etc., etc... that are not a matter of indentation. How can we keep enforcing them (BTW, they do not have PMD equivalents)

@gnodet
Copy link
Contributor Author

gnodet commented Apr 19, 2020

Not sure what you refer to exactly here. I think the formatting takes into accounts all style related rules, spaces, braces, indentation, etc... You may have seen the changes in indentation, but that only means all other are the same.

Checkstyle also has metrics - such as max. lines per method, max. lines per file, max. parameters per method, enforce @Override annotation, rules for variables naming, visibility fields orderings, etc.,
etc., etc... that are not a matter of indentation. How can we keep enforcing them (BTW, they do not have PMD equivalents)

Ok, we can easily keep those, I don't have any problems with that. The ones that are problematic to me are mostly the styling ones.
I'll add a commit to bring them back.

@lgoldstein
Copy link
Contributor

Ok, we can easily keep those, I don't have any problems with that. The ones that are problematic to me are mostly the styling ones.

I can live with that - let's keep the metrics, naming conventions, placement rules and comment out the rules that have to do with indentation

@gnodet gnodet force-pushed the SSHD-978 branch 2 times, most recently from 0fbca5d to c05f4ca Compare April 19, 2020 22:21
@gnodet gnodet merged commit 808cdea into apache:master Apr 20, 2020
@gnodet gnodet deleted the SSHD-978 branch April 20, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants