[xwiki-devs] [RFC] Review of checkstyle rules.
Hi all, This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested. All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team. Changes I would like to make: #1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level. #2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently. #3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule. I'd like to hear comments on these proposed changes. Thanks, Caleb
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable. On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274 More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9 -- Sergiu Dumitriu http://purl.org/net/sergiu
Hi, I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false Thanks -Vincent On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 12 Sep 2014 at 18:38:42, [email protected] ([email protected](mailto:[email protected])) wrote:
Hi,
I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove
- Commons violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.commons:xwiki-commons - Rendering violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.rendering:xwiki-rendering - Platform violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.platform:xwiki-platform Thanks -Vincent
- Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community. There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other. It's kind of backwards to agree on the rules before agreeing on the tool first. On 09/12/2014 12:38 PM, [email protected] wrote:
Hi,
I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu
On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”. Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools). Thanks -Vincent
On 09/12/2014 12:38 PM, [email protected] wrote:
Hi,
I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 09/12/2014 12:52 PM, [email protected] wrote:
On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org
So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”.
Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools).
Two question I have about Sonar: 1. Can it fail the build when I run mvn install locally? I don't want to have to wait hours to find out if I have all the spaces in order. If it's only an online asynchronous tool, we'll get in the same situation as with our CI failures, we'll pay attention to failures for a few weeks, then most devs will give up and check the build status only when pinged or when it's a BuildFixingDay. 2. Does it run faster or slower than the Checkstyle plugin? If slower, by how much? The build already takes too much time for devs to have the patients to build more than one submodule before committing, we don't want devs to skip running checks locally completely. -- Sergiu Dumitriu http://purl.org/net/sergiu
On 09/12/2014 01:01 PM, Sergiu Dumitriu wrote:
On 09/12/2014 12:52 PM, [email protected] wrote:
On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org
So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”.
Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools).
Two question I have about Sonar:
1. Can it fail the build when I run mvn install locally? I don't want to have to wait hours to find out if I have all the spaces in order.
If it's only an online asynchronous tool, we'll get in the same situation as with our CI failures, we'll pay attention to failures for a few weeks, then most devs will give up and check the build status only when pinged or when it's a BuildFixingDay.
2. Does it run faster or slower than the Checkstyle plugin? If slower, by how much? The build already takes too much time for devs to have the patients to build more than one submodule before committing, we don't want devs to skip running checks locally completely.
It's funny that the Sonar Maven Plugin uses Checkstyle... And has known violations in the latest release... http://mojo.codehaus.org/sonar-maven-plugin/checkstyle.html -- Sergiu Dumitriu http://purl.org/net/sergiu
On 12 Sep 2014 at 19:02:38, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
On 09/12/2014 12:52 PM, [email protected] wrote:
On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org
So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”.
Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools).
Two question I have about Sonar:
1. Can it fail the build when I run mvn install locally? I don't want to have to wait hours to find out if I have all the spaces in order.
You need to have sonar.xwiki.org up. AFAIU you execute mvn sonar:sonar and if you have installed this sonar plugin it’ll fail the build: http://docs.codehaus.org/display/SONAR/Build+Breaker+Plugin
If it's only an online asynchronous tool, we'll get in the same situation as with our CI failures, we'll pay attention to failures for a few weeks, then most devs will give up and check the build status only when pinged or when it's a BuildFixingDay.
Yes I agree.
2. Does it run faster or slower than the Checkstyle plugin? If slower, by how much? The build already takes too much time for devs to have the patients to build more than one submodule before committing, we don't want devs to skip running checks locally completely.
I think the problem is that sonar:sonar will run a ton of other things so we need to verify if we can find a way to have several execution paths: - one path for developers when they want to quickly check locally if their code that they want to push have any violations. Note to self: verify if this can work on uncommitted code. - another path for the CI, that executes less often to generate the full sonar.xwiki.org dashboard with TPC computations and more TBH I don’t know how well this will work out at this point. We need to try it out for real and see how it works. Now we could imagine: - having simple fast rules in our build that execute locally - having more complex rules (like TPC% less than previous values) that run on CI only Thanks -Vincent
Hi, Le 12 sept. 2014 19:11, "[email protected]" <[email protected]> a écrit :
On 12 Sep 2014 at 19:02:38, Sergiu Dumitriu ([email protected](mailto:
[email protected])) wrote:
On 09/12/2014 12:52 PM, [email protected] wrote:
On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead. The fact that there's healthy competition is good, not bad, for a project. And
we're
not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org
So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”.
Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools).
Two question I have about Sonar:
1. Can it fail the build when I run mvn install locally? I don't want to have to wait hours to find out if I have all the spaces in order.
You need to have sonar.xwiki.org up. AFAIU you execute mvn sonar:sonar and if you have installed this sonar plugin it’ll fail the build: http://docs.codehaus.org/display/SONAR/Build+Breaker+Plugin
But you can't build offline and get your checks executed anymore, as it will always fail to reach sonar.
If it's only an online asynchronous tool, we'll get in the same situation as with our CI failures, we'll pay attention to failures for a few weeks, then most devs will give up and check the build status only when pinged or when it's a BuildFixingDay.
Yes I agree.
2. Does it run faster or slower than the Checkstyle plugin? If slower, by how much? The build already takes too much time for devs to have the patients to build more than one submodule before committing, we don't want devs to skip running checks locally completely.
I think the problem is that sonar:sonar will run a ton of other things
That added to network latencies for pushing the metrics to sonar added to the fact that you need access to sonar database, not only http (from your computer I mean), which can be a problem for spread teams. so we need to verify if we can find a way to have several execution paths:
- one path for developers when they want to quickly check locally if their code that they want to push have any violations. Note to self: verify if this can work on uncommitted code. - another path for the CI, that executes less often to generate the full sonar.xwiki.org dashboard with TPC computations and more
TBH I don’t know how well this will work out at this point. We need to try it out for real and see how it works.
Basically sonar only aggregates and consolidates other plugins rules, and adds some in the process. Personally I find it great for daily dashboards, readable even by not-so-technical people, but for failfast builds it's a bit heavy unless you have a performant infrastructure. I use it only in nightly builds and for releases, for failing the build I configured the same rules in checkstyle, pmd and findbugs. Rulesets can be exported from sonar, but with some rework for findbugs. That being said, the 3 plugins alone are not so fast either... :D and findbugs is great but their maven plugin...
Now we could imagine: - having simple fast rules in our build that execute locally - having more complex rules (like TPC% less than previous values) that
run on CI only
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 12 Sep 2014 at 18:46:06, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
A tool still being maintained and released isn't dead.
Actually you’re right. It has been unmaintained for a long time but apparently last year 2 committers picked it up and moved it to GitHub to start a new life for it. Thanks -Vincent
The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community.
There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other.
It's kind of backwards to agree on the rules before agreeing on the tool first.
On 09/12/2014 12:38 PM, [email protected] wrote:
Hi,
I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
I object to this process. The question I raised is about 2 existing codestyle rules which we currently break the build over. I would like them removed and I made an argument why neither of them is defensible. In addition I believe we *should* break the build over a third rule which is not currently existing but I feel I have a justification for it. I think you've hijacked the thread by replacing the simple question of whether these three policies are justifiable with a question of whether technology Y is a better fit for our needs than technology X. If you feel that changing build/validation technology is important, I invite you to begin a mail thread and I promise I won't hijack the discussion with talk about software licenses, programming languages or whathaveyou. Thanks, Caleb On 09/12/2014 06:38 PM, [email protected] wrote:
Hi,
I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) wrote:
I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable.
On 09/12/2014 04:15 AM, Caleb James DeLisle wrote:
Hi all,
This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested.
All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team.
Changes I would like to make:
#1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level.
+0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule.
#2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently.
+1. All public code should be thoroughly documented, and private code should only document what's not obvious.
#3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule.
+1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274
More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9
-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
participants (4)
-
Caleb James DeLisle -
Jeremie BOUSQUET -
Sergiu Dumitriu -
vincent＠massol.net