overall, the i believe the issue of whitespace usage is a matter of personal style, and should not be subject to strict rule enforcement; as long as i can use CSOFF to disable rules on source files i create, then i can accept rules which encode different usage patterns;
i believe it is more important to preserve the style of the original author of a given source file rather than attempt to follow an arbitrary usage pattern in this regard; i don't mind using rules that differ from mine when those source files were authored by those different usage patterns; but i do not agree with enforcing them in my own style for a variety of reasons: - it slows me down - it makes it harder for me to read my own code, since i am accustomed to reading my style ideally, i believe you should craft rules that are sufficiently flexible in the area of personal style choices that accommodate all of our preferences; however, if it is acceptable to deal with exceptions using CSOFF, etc., then that would be sufficient for me On Mon, Feb 6, 2012 at 10:09 AM, Vincent Hennebert <[email protected]>wrote: > Hi Glenn, > > Thanks for taking the time to look at this. Looks like we should be able > to reach a consensus without too much difficulty. > > On 03/02/12 21:20, Glenn Adams wrote: > > which version of checkstyle are you using? there are two errors in > parsing > > the proposed checkstyle file with 5.1; > > > > <!-- <property name="ignoreEnhancedForColon" value="false"/> --> > > <!-- <module name="OneStatementPerLine"/> --> > > > > > > once i fixed the checkstyle file to work with 5.1, i see that 4935 and > > 31935 new warnings/errors are introduced in trunk and in my i18n > branches, > > respectively; clearly, this is going to require a large amount of editing > > to allow use of the proposed rules; > > Like I said most of them are purely about syntax and are easily solved > with a code formatting tool. Obviously I’m happy to run such a tool on > your own Git branches and submit a patch if that can help you. > i prefer not to use automatic tools to make fixups in this case for the reasons that chris outlined > > many of the new errors I notice (in both trunk and my i18n branches) have > > to do with whitespace before or after '(', ')', and cast operations; i do > > not agree with enforcing the presence or absence of whitespace around > these > > constructs; i happen to always use whitespace before and after parens, > > e.g., the following should produce no checkstyle warning: > > > > public int foo ( int a, int b, int c ) { > > return bar ( a, b, c ); > > }; > > I’d rather keep the rule, as it enforces standard Java style that will > be easily recognized by any Java developer. I also find the variation in > the use of whitespace to be one of the most distracting things when > reading code. > > That said, if that really bothers you I would be ok with relaxing the > rule, except for the whitespace between a method call and the left > parenthesis, to make it clear that it’s a method call. > i prefer my style of using whitespace; if you are not insisting that no CSOFF declarations appear in source code, then I can accept your proposal, provided I can use CSOFF to disable this rule for source files that i create (for those i didn't create, i can adhere to the rule) > > i would like whitespace after '{' and before '}' in an array > > initialization, e.g., both of the following should be permitted: > > > > int[] a = new int[] { 1, 2, 3 }; > > int[] a = new int[] {1, 2, 3}; > > Yep, no problem. > > > > i would like SimplifyBooleanReturn to be removed; > > Hmmm. Ok. > > > > i would like whitespace after BNOT produce a warning, e.g. both ! foo and > > !foo should be accepted without warning; > > I’d keep the rule. Allowing a standalone exclamation point is too > dangerous I think. Too easy to miss. > again, if you don't mind me using CSOFF in source files I author, then I can accept > > > > i would like whitespace after DOT operator to be permissible, e.g., both > > x.y and x . y should be permitted; > > Why? Note that it’s possible to break the line before the dot. when i cast an object reference then invoke a method, i like to use the following whitespace: ( (Foo) obj ) . doit ( ... ) again, if you don't mind me using CSOFF in source files I author, then I can accept > > > i would like empty blocks to be permissible, e.g., the following should > be > > permitted: > > > > if ( test ) { > > /* TBD - handle test is true */ > > } else { > > /* TBD - handle test is false */ > > } > > I find that it’s just clutter, but I don’t really mind. > the issue is i would like to put comments into blocks that are otherwise empty; this is useful as a reminder to me as a coder that i may need to do something for those blocks in the future that make them non-empty > > > > i would like the arbitrary line length rule to be removed; i do not agree > > to 110 line length; or if you insist, i could accept 150; > > I’m afraid I’m not happy to go any higher than 110. Pascal actually made > a good point by saying that there is only a certain number of tokens > that a human being can grasp on one line. > > Also, having to scroll horizontally is a real pain and greatly impedes > the readability of code. And if the editor is set up to automatically > wrap long lines then this ruins the indentation, which is not any > better. > my screen width and editor (emacs) and font size allows me to use 150 characters without scrolling; i think this is just as reasonable as 110; frankly both of these are arbitrary; rather than spending time arguing about arbitrary limits, i would prefer simply removing this rule/limit, and leaving it to peer review > > > > i do not agree with including MultipleVariableDeclarations rule; i > > routinely define multiple local variables in one statement, e.g., int x, > y; > > I’d really rather keep the rule. Variables should be used (i.e., > initialized) as soon as they are declared anyway. Otherwise there is > a risk that with time, the variable declaration appears further and > further away from where it is used. > i prefer being able to use multiple declarations in one statement when it makes sense; the language allows it, so why should we impose an arbitrary rule to prevent it? again, if you don't mind me using CSOFF in source files I author, then I can accept > > > > i do not agree with requiring LocalFinalVariableName to match > > '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'; > > instead, it should continue to match the currently used > > pattern "^[a-z][a-zA-Z0-9]*$"; > > Unless I missed something this is the default in Checkstyle 5.5. > i got a warning under Checkstyle 5.1; so i guess the default has changed; in order to avoid being subject to this change, i suggest it be made explicit > > > > why are there two NoWhitespaceAfter rules? > > > > <module name="NoWhitespaceAfter"> > > <property name="tokens" value="ARRAY_INIT"/> > > </module> > > <module name="NoWhitespaceAfter"> > > <property name="allowLineBreaks" value="false"/> > > <property name="tokens" > > value="BNOT,DEC,DOT,INC,LNOT,UNARY_MINUS,UNARY_PLUS"/> > > </module> > > Because one rule allows a line break after the token, the other not. But > anyway, following your comment we would remove the former one. > > ok > > > if you fix the above problems, then i will re-run on trunk and my i18n > > branch to check if there are any other issues that need to be resolved; > > Let me know what you think. Hopefully others will also speak up to share > their views. > > Thanks, > Vincent > > > > On Fri, Feb 3, 2012 at 10:45 AM, Vincent Hennebert <[email protected] > >wrote: > > > >> Hi All, > >> > >> it is well-known that people are not happy with the Checkstyle file we > >> have in FOP. And there’s no point enforcing the application of > >> Checkstyle rules if we don’t agree with them in the first place. > >> > >> I’ve finally taken on me to create a new Checkstyle file that follows > >> modern development practices. I’ve been testing it on my own projects > >> for a few months now and I’m happy with it, so I’d like to share it with > >> the community. The idea is that once we’ve reached consensus on the > >> Checkstyle rules we want to apply, we could set up a no warning policy > >> and enforce it by running Checkstyle in CI. > >> > >> I’m also taking this as an opportunity to propose that we adopt a common > >> Checkstyle policy to all the sub-projects of XML Graphics. So once we’ve > >> agreed on a set of rules we would apply them to FOP and XGC immediately, > >> and eventually also to Batik, and keep them in sync. > >> > >> We would also apply the rules to the test files as well as the main > >> code. Tests are as important as the actual code and there is no reason > >> why they shouldn’t be checked. > >> > >> It is likely that the current code will not be compliant with the new > >> rules. However, most of them are really just about the syntax, so > >> I believe it should be fairly straightforward to make the code at least > >> 90% compliant just by applying Eclipse’s command-line code formatter. > >> > >> Please find the Checkstyle file attached. It is based on Checkstyle 5.5 > >> and basically follows Sun’s recommendations for Java styling with a few > >> adaptations. What’s noteworthy is the following: > >> > >> • Removed checks for Javadoc. What we want is quality Javadoc, and that > >> is not something that Checkstyle can check. Having Javadoc checks is > >> counter-productive as it forces us to put {@inheritDoc} everywhere, or > >> to create truly useless doc like the following: > >> /** > >> * Returns the thing. > >> * @return the thing > >> */ > >> public Thing getThing() { > >> return thing; > >> } > >> This is just clutter really. I think it should be left to peer review > >> to check whether a Javadoc comment is properly written, or whether the > >> lack thereof is justified. There’s an excellent blog entry from > >> Torsten Curdt about this: > >> http://vafer.org/blog/20050323095453/ > >> • Removed check for file and method lengths. I don’t think it makes > >> sense to define a maximum size for files and methods. Sometimes > >> a 10-line method is way too big, sometimes it makes sense to have it > >> reach 20 lines. Same for files: it’s ok to reach 1000 lines if the > >> class contains several inner classes. If it doesn’t, then it’s > >> probably too big. I don’t think there is any definite figure we can > >> agree on and blindly follow, so I think sizes should be left to peer > >> review. > >> • However, I left the check for maximum line length because unreasonably > >> long lines make the code hard to follow. I increased it to 110 > >> though to follow the evolution of monitor sizes. But as Peter > >> suggested me, we probably want to keep it low in order to make > >> side-by-side comparison easy. > >> • I added a check for the order of imports; this is to reduce noise in > >> diffs when committing. I think most of us have configured their IDE to > >> automatically organise imports when saving changes to a file. This is > >> a great feature because it allows to keep the list of imports > >> up-to-date. But in order to avoid constant back and forth changes when > >> different committers change the same file, I think it makes sense that > >> we all have the same configuration. I modeled this list after > >> Jeremias’ one, that I progressively inferred from his commits. > >> > >> Please let me know what you think. I’m inclined to follow lazy consensus > >> on this, and apply the proposed changes if nobody has objected within > >> 2 weeks. If anybody feels that a formal vote is necessary, feel free to > >> say so. > >> > >> Thanks, > >> Vincent > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: [email protected] > >> For additional commands, e-mail: [email protected] > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
