Hi all,
I tried to apply this checkstyle to Papyrus plugins.
As we could imagine, the first analysis reach the exceed limit. :(
I picked a report printscreen just before the oversize limit to have
an overview.
I guess we could reach the 1M of warnings, (were about 24% is
format related , and no filter at all even on generated sources)
One third has been analysed (result below ) and the top 10 of
non-format-related warnings are quite similar to papyrus RT:
Regards
Céline
Le 18/03/2016 10:38, Céline JANSSENS a
écrit :
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
|
|
_______________________________________________
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
|
|
|