On Mon, Aug 7, 2017 at 12:16 PM, Gilles <gil...@harfang.homelinux.org> wrote:
> Hello. > > On Fri, 4 Aug 2017 21:17:43 +0300, Allon Mureinik wrote: > >> We had a similar discussion about Configuration. >> >> Personally, I'm all for enforcing checkstyle during the validate phase, >> but >> we couldn't reach a consensus about it there: >> http://www.mail-archive.com/dev@commons.apache.org/msg58573.html >> >> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter <krich...@posteo.de> >> wrote: >> >> Hi, >>> While working on a [small >>> contribution](https://issues.apache.org/jira/browse/MATH-1426) I noticed >>> that there's a checkstyle setup which is run in a reporting phase of >>> Maven which might be skipped by most developers and isn't used on Travis >>> CI. >>> >> > Well, running > $ mvn site > and checking the reports, is one of the (unwritten?) rule a contributor > to Commons will be told about. ;-) > I think unwritten rules are worth as much as the paper they're written on. Running "mvn site" is a great practice, but if we expect people to do so, it should be stated very clearly in the contributor's guide. > I suggest to move this phase to the validate phase of Maven which >>> runs before the compile and test phase and in case of failure forbids >>> the invoker to build the project successfully. >>> >> > Forcing style even before the compiler has the chance to warn about > invalid code looks a bit strong. > > Therefore most >>> contributions will like they're intended too without the need of extra >>> communication. >>> >> > If the above rule can be improved over, fine, but running CheckStyle > takes time (e.g. wrt to the compilation of a fix being worked on); so > it should be "mandatory" only before submitting a contribution. > > Perhaps, we should have a maven profile "-P check-requirements" > which contributors _must_ run before providing a PR or attaching a > patch to JIRA. > That profile would thus contain your suggestion. > > >>> The downside is that new (and eventually old) devs might be annoyed at >>> some point, especially if they frequently work on different projects >>> with different styles. >>> >> > Indeed, hence my suggestion to not change the usual workflow but to > advertize that contribution will be taken into account only if they > pass the requirements. > I think there's two needs we should answer here. First, maintainers shouldn't have to run any additional step in order to decide whether a patch is worthy. They should look at the code and commit message(s), adn see if they have their merrit. Any technical requirement (compilation, code style, test coverage, etc) can and should be handled by automatic tools (namely: CI, or to be more specific, Travis CI we're using on GitHub). Unless it takes hours upon hours to run (which it shouldn't), the fact that it takes time is inconsequential. You submit your patch, and once CI have verified that it's up to standard (usually within seveal minutes), a maintainer can take a look and judge the subtance of the patch. This way, maintainers don't waste their time on boilerplate commenting. Second, contributors need to be made aware of the expectations. I.e., a contributor should know that if he or she runs command line X (regardless of whether it's "mvn install -Pcheckstyle", "mvn site" or even "mvn giles"), there are pretty good chance that the CI will also pass, assuming there isn't some problem that only occurs on an alternative jdk/platform. > > >>> I can take over the move to the validate phase which is 10 lines >>> insertion/deletion in pom.xml, but not the definition of code style >>> rules which are common for the project because I don't know them. Doing >>> this change reveals about 400 issues of which > 95% are related to >>> missing or errornous Javadoc which is worth having a look at, but might >>> be postponed by deactivating the rule for now. Then you need to discuss >>> code style rules, because some, like the ones in the issue linked above, >>> aren't covered yet. >>> >> > For "Commons Math", there is a custom "checkstyle.xml" (in the top > directory of the code repository). > When running "mvn site", CheckStyle currently reports 1 error. > > The numerous errors you see (but which I do not) might be in the "test" > part of the source repository. [Historically, code style there was much > less emphasized (and perhaps checking it is disabled by in "mvn site").] > > Best regards, > Gilles > > >>> -Kalle Richter >>> >>> >>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
