On Mon, Aug 7, 2017 at 2:36 PM, Gilles <gil...@harfang.homelinux.org> wrote:
> On Mon, 7 Aug 2017 14:08:45 +0300, Allon Mureinik wrote: > >> 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. >> > > Perhaps this is the document to update: > https://commons.apache.org/patches.html > ? > > > 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. >> > > Less work for the maintainers is good. :-) > > By "taking time" I meant that validating should not be enforced when > calling "mvn compile" or "mvn test". > > 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. >> > > So IIUC, it would be necessary and sufficient to update > * the travis config > * the above web page > Sounds like a good start. Every commons project also has a CONTRIBUTING.md file that's autogenerated by running mvn commons:contributing-md. It should either also include the same guidelines (whatever we decide they should be), or add a link to the aforementioned page. These pages, BTW, already state that you should "Run all the tests with mvn clean verify to assure nothing else was accidentally broken." So I think adding verifications like checkstyle/findbugs/rat to the verify phase could be a good, balanced approach. > > Regards, > Gilles > > > >>> >>> 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 > >
