commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Allon Mureinik <murei...@gmail.com>
Subject Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])
Date Mon, 07 Aug 2017 11:49:16 GMT
On Mon, Aug 7, 2017 at 2:36 PM, Gilles <gilles@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 <gilles@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 <
>>>> krichter@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message