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 11:36:43 GMT
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

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