commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])
Date Mon, 07 Aug 2017 13:09:06 GMT
On Mon, 7 Aug 2017 14:49:16 +0300, Allon Mureinik wrote:
> 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.

+1

I guess that someone knowledgeable should add those calls in the 
"parent"
of the Commons projects.

Regards,
Gilles

>
>
>>
>> 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
View raw message