Hi Céline. I do understand the rationale for having checkstyle rules and a common format, and I understand that they are not blocking, but the main issue why I started this thread is that some of those checkstyle rules are inconsistent with the formatting preferences, and that should not be the case. We need to either change the formatting preferences or change the checkstyle rules, because it doesn't make sense that the auto-format results in code that violates the checkstyle rules, like the example with enums that I listed.
Sure, it is not blocking, but it is a big pain for me, because we didn't agree to these rules at the beginning of the project and as a result, the style used by codegen and xtumlrt is very different than the one in tooling. I've been updating these, but it is taking me a long time, and in some cases it is quite problematic, and error prone. I mentioned the multi-return as an example because it requires refactoring code, and I already made a mistake with one such refactoring.
Also, take a look at what one of our files looks like in the editor:
It is a bit ridiculous. In fact, formatting oeprt.codegen.cpp.rts/UMLRTRuntime.java and the whole oeprt.codegen.lang plugins just strikes fear in me. I already reformatted UMLRTRuntime.java but it still has a gazillion warnings because of missing javadocs, and the reason is that that class has a lot of attributes. Which is one of the checkstyle rules that I don't like: very often it is obvious from the attribute name and type what it is.
Anyway, In order for me to work with the files that are not already formatted, I need to reformat, which includes adding all the javadocs, which takes quite a bit of time, so if I need to do anything before a file is formatted, I need to disable checkstyle for the project, and disable that format on save action as well, and then ignore the updated .project, and .prefs when pushing to gerrit, and that becomes a pain when switching branches.
I'm not saying that we should get rid of checkstyle or autoformat at this point, but I think we should revisit the rules and be a bit more flexible with some of them, and at the very least, make sure that the formatting preferences and the checkstyle rules are not mutually inconsistent.
Yes, the purpose of the Checkstyle implementation is understood and, I hope, appreciated. However, after having let the current rule set soak for a few months, I think it is appropriate that we take a second look to see whether we are really getting the benefit that want from all of these rules. That’s a quite normal evaluation process. The main concern that I have right now is that I have a workspace in which very nearly every file has a yellow bang decoration on it, so I cannot tell where there are problems that I might need to worry about. I can’t see the trees for the forest. 😉 And it suggests to me that perhaps we have actually standardized on a slightly different style than what our rules suggest.
The Checkstyle rules have been discussed along with Rémi as a
basis. In order to have all the same way to code, and to have an
homogeneous code.
Several rules can be different from some habits, but the problem
with the habits is that they are all differents ;)
The formatter should give you the standard to be used.
None of the rules is blocking ! So if you have a really good reason
to not follow checkstyle indication, please document your
decision.
Nevertherless, if there are inconsistancies between the Formatter
and the Checkstyle, then we should obviously make it sync.
About the multi return...
This point has been deeply discussed before to decide to prefer a
single return statement.
In a maintainability point of view and in terms of debugging,
the single return statement is preferable.
I know that a lot of existing sources already use multiple return
statements. That is why we keep this rule as a warning and not as
an error.
The purpose is to improve the code formatting as soon as a source
is modified.
For long term, that should make the code stronger, homogeneous, and
maintainable.
Best regards
Céline
Le 21/06/2016 à 18:45, Christian Damus
a écrit :
Hi, Ernesto,
If we change the formatter profile, that should be done in
the org.eclipse.papyrusrt.umlrt.core bundle, which the Oomph
master for all of the project settings. From there, the Oomph
project configuration tooling will push the change to all of the
other projects. So, you don’t have to worry about changing
all of the settings files (Oomph does).
I would like to see a lot of changes in the Checkstyle rules,
too. I haven’t got time to make a list now, but we should
discuss it as a team (maybe in a Google sheet). There are so
many warnings in clean code that the current checkstyle profile is
basically useless.
I'm reformatting the code generator to conform to the
checkstyle rules introduces, but I have found a few problems. In
particular, it looks like there are some inconsistencies between
what the formatting does and what checkstyle approves. For example,
I have an enum with several entries which is formatted in two
lines, but checkstyle complains that the first line is too
long.
There are other problems as well. I consistently run
into a warning saying that only one return statement is allowed per
method. This feels quite stringent.
Or the requirement that every attribute has a
javadoc.
I was wondering if we could relax some of these
rules.
I see that the the checkstyle rules are in releng/oeprt.oomph/checkstyle. But what if we
wanted to change some of the formatting rules? Would we have to
change every single .settings/org.eclipse.jdt.core.prefs
file?
(I'd like to be able to write each enum entry in a
separate line without checkstyle complaining and without the
autoformat putting them in one line.)