Re: [papyrus-rt-dev] Checkstyle and formatting inconsistencies

[Ernesto Posse <eposse@xxxxxxxxxxxxx>]

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.

On Wed, Jun 22, 2016 at 7:49 AM Christian Damus <give.a.damus@xxxxxxxxx> wrote:

Hi, Céline,

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.

Cheers,

Christian

On 22 June, 2016 at 04:22:51, Céline JANSSENS (celine.janssens@xxxxxxxxxxx) wrote:

Hi All,

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.

Christian

On 21 June, 2016 at 11:14:32, Ernesto Posse (eposse@xxxxxxxxxxxxx) wrote:

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.)

--

Ernesto Posse

Zeligsoft

_______________________________________________
papyrus-rt-dev mailing list
papyrus-rt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/papyrus-rt-dev

_______________________________________________
papyrus-rt-dev mailing list
papyrus-rt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/papyrus-rt-dev

--

	Céline JANSSENS Software Engineer +33 (0)2 44 47 23 23
	Mail : cej@xxxxxxxxxxx
6 rue Léonard De Vinci - BP 0119 - 53001 LAVAL Cedex - FRANCE www.all4tec.net

Garanti sans virus. www.avast.com

_______________________________________________
papyrus-rt-dev mailing list
papyrus-rt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/papyrus-rt-dev

_______________________________________________
papyrus-rt-dev mailing list
papyrus-rt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/papyrus-rt-dev