Hi, Céline,
See some more replies in-line, below. I’ve snipped out some material to make this a bit more manageable. 😉
On 18 March, 2016 at 05:38:26, Céline JANSSENS (celine.janssens@xxxxxxxxxxx) wrote: 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 !
That’s odd. It sounds to me like a bug in Checkstyle. On every platform, regardless of convention, the line is ultimately terminated by LF (which is the second of the CRLF pair on Windows; I assume nobody uses the MacOS 9 and earlier CR convention any more). I would have hoped that the recent correction of EOLs in git would have normalized the situation so that you would see consistent results. I don’t know now what to suggest, either. I don’t like the idea that Checkstyle configurations that are meant to be shared on multiple platforms make one choose a platform-specific EOL to scan for.
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.
Yes, that’s true, but does the balance of trade-offs make sense when it is as much effort for a developer to write a redundant doc comment as to add a warning filter in each trivial case?
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.
That’s a problem, IMHO. It is fairly common to use static final fields for mutable objects that must not be replaced (which is why they are final). I would argue that only true constant values must use the all-caps convention.
- 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...
I strongly dislike putting constants into interfaces when they are not intended as an API of the interface, usually as values for arguments of the interface’s operations that the contract recognizes. A constant that some code uses internally should be declared in the class that uses it; if another class can re-use it, it can do so with the same negligible cost of static field access as in any other case.
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" />
Yes, a lot of analysis tools should be steered away from generated code most of the time. That’s good. What I was thinking of specifically is the configuration of Checkstyle to run in the developer workbench, not in the build. A lot of these validation tools are provisioned on a per-project basis. For example, the PDE API Analysis is a project builder and its exception filters are stored in the project; each project maintains its own filters. Can generic Checkstyle filters such as your src-gen example be defined in one project (say, org.eclipse.papyrusrt.umlrt.core) and shared by all other projects in the workspace?
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.
That’s just the point I disagree on, though. “Code style” is about consistency, readability, maintainability, and comprehension of the code. How does giving labels to simple self-describing strings like “” and “,” improve any of those factors? For many things it makes sense to me, for example keys in look-up tables, bundle IDs, message patterns, regular expressions; these are all strings that have more abstract meaning. But some strings have an intrinsic absolute value that speaks for itself. I suppose if these cases can individually be filtered from the Checkstyle report, then I’d be satisfied with that.
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.
Indeed, that is always the delicate balance in a situation like this. We have to hope that the tools make it so easy that we don’t perceive compliance as an effort. Quick-fixes help, but constant monitoring that trains us not to deviate in the first place is better. That’s why I want to be sure that we can implement Checkstyle painlessly in the developer workspace for everyone, also, not just in the build.
|
