Thank you Christian for your feedback !
My answer below...
Le 17/03/2016 19:18, Christian Damus a
écrit :
Hi, Céline,
Thanks very
much for this report. It is quite informative to our
discussion.
Please see
some comments in-line, below.
On 17 March, 2016 at 13:36:19, Céline
JANSSENS (celine.janssens@xxxxxxxxxxx)
wrote:
Hi all,
Here is the state of art of the current Checkstyle on
Papyrus RT project.
As shown on the printscreen the most frequent warnings
are:
1) In the Javadoc the first sentence should end with
a period (meaning ., ? or ! )
This could seem a detail, but it makes the
difference. Your Javacode must look as professionnal as
possible to allow further automated documentation and a
professionnal aspect.
But I agree , for the existing files it is a lot of
time consuming to modify if. So let's this warning for
the next times...
I concur with the thinking behind this. I much prefer not to
have any doc comment at all than trivially auto-generated
comments and malformed structures that don’t account for the
basic HTML formatting rules and are only readable (sometimes
barely) in the source editor! We see a lot of both in the
Papyrus repo.
Java Auto-Doc is not a good practice I agree with you, empty shell
comment is completely meaningless.
2) File does not end with a new line
This may changed depending on the "system". I guess
if I select "lf" for the new line check it should
decrease this warnings. But we could meet this warning
on the Linux file.
Does anyone have an idea of what is the proper
ends of file to update the the check accordingly ?
This isn’t about the platform line ending convention that has
recently been a git-oriented discussion point, but literally
just the last line of the file. Diff tools often complain or
show some kind of scary marker when the last line of a file
that contains text is not terminated by some kind of newline
(even if it’s not the platform’s conventional terminator).
Perhaps it suggests truncation of a file, that there’s stuff
missing at the end …
In my experience, most if not all source files generated by
EMF have the last line (the closing brace of the class
declaration) unterminated. So that should generate a lot of
warnings.
I put "lf" instead of system an it decrease the number of warning
from 300 to 60... The remaining warning seems to be linux file.
I don't know how to manage this !
3) Missing Javadoc
The Javadoc helps contributors to understand the
different API properly. A clear Javadoc is necessary
to help further contributors to understand your
code, otherwise, your code will never last and
someone else would prefer re-develop it.
Yes, but there are also trivial cases that don’t warrant
documentation. A well-named getter method often doesn’t
need anything, for example. Sometimes writing something
just for the sake of writing it doesn’t serve anyone’s
interest.
I do agree, however, this check highlight a missing javadoc which in
most case is really missing ! It would become difficult to define
the bounds of which method should be commented which not... And
could be an open door to not comment anything.
We should keep in mind that these are "warnings" and can be
overrided.
4) Naming Convention:
Due to several cases, i.e:
Does Checkstyle distinguish between actual constant
immutable value types and things that are mutable?
For example, a public static final field holding a
Map is anything but constant, and we have boatloads of
those in Papyrus.
I don't think so, on checkstyle website a constant is a Static
final field.
To be defined.... ;) In the naming Convention, the format is defined
by RegExp and for now on the max length of a variable is defined to
28 characters . It is also possible to define the legnt of a Method
Name, Type name, parameters name, Abstract Method Name, ...
Missing Abstract in front of Abstract
classes
This is an interesting one. What’s the rationale for
that? Repeating an arbitrary prefix like this makes names
all look indistinguishable (it’s one of the reasons why I
hate the “I” prefix for interfaces, but I just have to
accept it in the Eclipse context …)
I understand your concern, but this is to keep an homogeneous code.
And with the prefix "I" (for interface ) ," Abstract" or even
suffix "Exception" or "Handler", everyone knows the role of a class
at a glance. As a lot of naming convention rules it doesn't make a
better code, but only make it clearer for the others.
BTW, what do you think about using Interfaces for constants only....
this is a rule I disable for Papyrus RT, because I know it's a
common habit in the Papyrus developers, even if it is not the role
of an Interface...
Of course this check can be improved by
adding some filter for the exceptions.
Can those filters be shared in one place, or would they
have to be configured in each project? (Oomph can
perhaps help if the latter)
There is another xml file that lists all the exceptions by selecting
the checks and the files on which warning should be supressed.
I already used it to filter all the src-gen file from umlrt folder:
<suppress checks="[a-zA-Z0-9]*"
files="org\.eclipse\.papyrusrt\.umlrt.*[\/\\]src-gen[\/\\].*\.java"
/>
5) Multi Return
This is quite used in the developers
habits to have several return statement in the
same method. However a single return statement
by method improves the maintainability and the
extensibility of the code.
It also helps a lot for the debug as only
one breakpoint is required to examine the
returned value.
I happen to be an adherent of the single-return
convention, but I hesitate to impose it on others
because I know that some people just don’t like much
nesting of conditionals and loops. How do others on
the team feel about this?
Depending on the opinion, this rule can be put as "info" severity
instead of warnings if it is too constraining.
6) Mandatory Default Constructor
This point could be discussed. But in my
point of view it is clearer to have always a
constructor, even if it is the default one.
Agreed. It is an excellent place to put a
breakpoint, and it makes it clear that the
developer’s intent is to provide a no-argument
constructor. I even always explicitly chain to
super() even if that would be implied, for the same
reason.
7) Do not use trailing comment on
the same line as a java instruction.
It is a very good habit to comment
your code to make it clear and structured.
But it is even clearer if the comment is
at a separate line to not reach a too long
line.
That’s interesting. I sometimes use an empty
comment to force line breaks where otherwise the
formatter would string stuff out on a line. And
in case statements, if-else, and other such
tightly repeated structures a concise end-of-line
comment might look more readable to me. Again,
I’d be interested to hear the opinions of others
on this.
8) The String "X" appears "Y"
times.
If a string is required it should be
extracted as a Constant, especially if
this string is present more than once in
your file;
Sure, unless it’s something like the empty
string (“”) which is an absolute literal value
or something else which likewise clearly
signifies its own meaning. There’s a difference
between, say, bundles IDs that are fairly
abstract and things like “,” used in a
string-splitting operation.
Yes in anycase, it is a good practice to avoid redundant string if
it means the same.
9) "X" hides a field
This means that for example:
protected int numberOfLine;
public void setNumberOfLine(final
int numberOfLine ){
...}
The parameter hides the field
because it has the same name in a
closer scope. This can be confusing.
As we are not using prefixed
variables, it is required to make the
difference.
Ordinarily, I would agree, but in the case of
a setter, it’s pretty obvious and even helps
to reinforce the purpose of the local variable
when it has the same name as the field.
And when the field is a wrapper of some kind,
like a Reference<T>, it can be nice to
unwrap it as a local variable of the same name
because in some sense it is the “same” thing.
10) Redundant modifier
This is when you add a public
modifier in an Interface which is
unnecessary.
Yes! That so bugs me; I don’t know why.
🙂 And “abstract”, “final” (for fields),
“static” (for nested interfaces), …
Of course Format errors can
be easily fixed with a proper
formatter.
This would be a motivator for the
re-formatting sweep that I was arguing
against on another thread. Hmm.
If we fix those top 10
warnings, we would fix 85 % of
the checkstyle Warning.
Wow, that’s impressive. Sounds like it’s
worth investing the time in fixing these.
Especially as I would suggest (according
to my comments above) that some of those
warnings might be fixed by filters or
disabling the rule altogether. 😉
BTW, do we have a similar set-up for
Checkstyle on the Papyrus project? It
would be nice to see consistency there,
perhaps even back-porting what is decided
for Papyrus-RT to Papyrus.
I could try to add the same checkstyle for Papyrus Project, and see
:)
The goal of this checkstyle is also to help developers to improve
the way they code in terms of team work. But we should keep in mind
that if the set of rules is too difficult to be followed, nobody
will do so, and it will be a waste of time.
What is your opinion ?
Best regards
Céline
Le
16/03/2016 18:55, SCHNEKENBURGER
Remi 211865 a écrit :
Hi all,
Thanks to
everyone for this fruitful
discussion! I could see
here a lot of proposals
and volunteers ;)
I would propose
the following things:
- Papyrus-RT
should reuse the Papyrus
default formatter, as this
would reduce work when
‘upgrading’ some code from
Papyrus-RT to generic
Papyrus. It is also very
similar to default
formatter AFAIR.
- Papyrus-RT
should reuse a checkstyle
configuration close to a
generic configuration, as
for example the one in
Sonar. I would prefer not
to get too far from
existing configurations,
and a simple one to begin
with. There is already a
basic one within Eclipse.
Céline proposes one, is
there one on Collaborative
modeling, Philip?
- The save action
could format the code and
do the basic cleaning
(import cleaning, etc.)
I like very
much the idea of
pre-commit hooks, because
usually the only respected
rules are the one that are
enforced ;) but I don’t
know here how it does fit
well with Eclipse, Egit
& command line users,
etc. I would prefer to
avoid manual actions to be
performed by developers to
setup the environment :
Oomph was typically
invented to reduce the
amount of time needed to
start committing on a
project while removing
manual tasks.
To have a
concrete plan:
- @Céline, can
you provide a report on
current Papyrus-RT code
base with a basic
checkstyle configuration
or the one you usually
use?
- @Philip, can
you propose also a
checkstyle configuration
file from collaborative
modeling?
- @Céline, can
you give an update to all
committers on the status
of the Sonar analysis job
on Papyrus-RT? Is this
possible to run this
analysis on each gerrit
contributions?
- @All, what
actions should be done on
Save?
- @All, do you
agree to reuse Papyrus
formatter?
- @Christian,
which settings would you
propose from the Papyrus
project on the project
.settings - JDT settings
and others?
Thanks,
Rémi
-------------------------------------------------------
Rémi
SCHNEKENBURGER
+33 (0)1 69 08
48 48
CEA Saclay
Nano-INNOV
Institut
CARNOT CEA LIST
 www.eclipse.org/papyrus
De : papyrus-rt-dev-bounces@xxxxxxxxxxx [mailto:papyrus-rt-dev-bounces@xxxxxxxxxxx] De la part de Ernesto
Posse
Envoyé : mercredi
16 mars 2016 18:14
À : papyrus-rt
developer discussions <papyrus-rt-dev@xxxxxxxxxxx>
Objet : Re:
[papyrus-rt-dev] Code
formatting and checkstyle
rules
So we
should set the "Save
Actions" individually and
commit those to the repo?
_______________________________________________
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 |
|
_______________________________________________
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
|
|
|
