[MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase

Karl-Philipp Richter
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. 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. Therefore most
contributions will like they're intended too without the need of extra
communication.

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.

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.

-Kalle Richter


signature.asc (465 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [MATH] Enforce run of checkstyle-maven-plugin in validate instead of site phase

Allon Mureinik
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@.../msg58573.html

On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter <[hidden email]>
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. 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. Therefore most
> contributions will like they're intended too without the need of extra
> communication.
>
> 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.
>
> 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.
>
> -Kalle Richter
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Gilles Sadowski
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@.../msg58573.html
>
> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter
> <[hidden email]>
> 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 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 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Allon Mureinik
On Mon, Aug 7, 2017 at 12:16 PM, Gilles <[hidden email]>
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@.../msg58573.html
>>
>> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter <[hidden email]>
>> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Gilles Sadowski
On Mon, 7 Aug 2017 14:08:45 +0300, Allon Mureinik wrote:

> On Mon, Aug 7, 2017 at 12:16 PM, Gilles
> <[hidden email]>
> 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@.../msg58573.html
>>>
>>> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter
>>> <[hidden email]>
>>> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Allon Mureinik
On Mon, Aug 7, 2017 at 2:36 PM, Gilles <[hidden email]> wrote:

> On Mon, 7 Aug 2017 14:08:45 +0300, Allon Mureinik wrote:
>
>> On Mon, Aug 7, 2017 at 12:16 PM, Gilles <[hidden email]>
>> 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@.../msg58573.html
>>>>
>>>> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter <
>>>> [hidden email]>
>>>> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Gilles Sadowski
On Mon, 7 Aug 2017 14:49:16 +0300, Allon Mureinik wrote:

> On Mon, Aug 7, 2017 at 2:36 PM, Gilles <[hidden email]>
> wrote:
>
>> On Mon, 7 Aug 2017 14:08:45 +0300, Allon Mureinik wrote:
>>
>>> On Mon, Aug 7, 2017 at 12:16 PM, Gilles
>>> <[hidden email]>
>>> 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@.../msg58573.html
>>>>>
>>>>> On Fri, Aug 4, 2017 at 7:16 PM, Karl-Philipp Richter <
>>>>> [hidden email]>
>>>>> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)? (Was: [MATH] Enforce run [...])

Karl-Philipp Richter
Hi,

Am 07.08.2017 um 15:09 schrieb Gilles:
> 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".
I wouldn't worry about the time consumption of the validation even if
it's run by every dev before very compilation since the sum of
conversation parts in patch/PR discussion in the form of "LGTM, except
for the indentation at line xy" - "OK, I fixed that now" - "Oh, no wait,
I forgot the trailing space at line yz" - ... - merged takes an infinite
more of time and energy.

Regarding the phase where checkstyle should be run I have some
additional thoughts to my initial post:

  * If running checkstyle will be enforced the only phase that makes
sense is `validate` because you don't won't to build something that's
invalid because it's somehow unlogical and a waiste of time if you don't
fail the build as early as possible. In order to avoid annoyance for
users who aren't used to fix checkstyle errors before being able to
build I'd suggest a profile with deactivated checkstyle which allows
that rather an putting checkstyle in a separate profile.
  * Running checkstyle in the site or any other reporting phase is in
Maven speak afaik "show what might be wrong with my build given the fact
that I consider it passing after compilation, unit and integration tests
passed" or "show me some statistics about style issues - 150, wow that's
12 less than last build".


signature.asc (465 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)?

Gilles Sadowski
Hello.

On Wed, 9 Aug 2017 00:20:00 +0200, Karl-Philipp Richter wrote:

> Hi,
>
> Am 07.08.2017 um 15:09 schrieb Gilles:
>> 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".
> I wouldn't worry about the time consumption of the validation even if
> it's run by every dev before very compilation since the sum of
> conversation parts in patch/PR discussion in the form of "LGTM,
> except
> for the indentation at line xy" - "OK, I fixed that now" - "Oh, no
> wait,
> I forgot the trailing space at line yz" - ... - merged takes an
> infinite
> more of time and energy.

I agree, but that is with respect to interaction with someone not used
to the coding style/rules; what I meant is that when doing one's "own"
work, one shouldn't have to wait for CheckStyle at every compilation,
when you know that you'll fix the missing doc _after_ fixing the code.

> Regarding the phase where checkstyle should be run I have some
> additional thoughts to my initial post:
>
>   * If running checkstyle will be enforced the only phase that makes
> sense is `validate` because you don't won't to build something that's
> invalid because it's somehow unlogical and a waiste of time if you
> don't
> fail the build as early as possible. In order to avoid annoyance for
> users who aren't used to fix checkstyle errors before being able to
> build I'd suggest a profile with deactivated checkstyle which allows
> that rather an putting checkstyle in a separate profile.

IIUC, that would be fine (since it takes care of the above scenario).

>   * Running checkstyle in the site or any other reporting phase is in
> Maven speak afaik "show what might be wrong with my build given the
> fact
> that I consider it passing after compilation, unit and integration
> tests
> passed" or "show me some statistics about style issues - 150, wow
> that's
> 12 less than last build".

That's what we've done up to now; and the number of errors is supposed
to be zero before a release.  But I agree that the risk of a lot of
work
for the RM would be reduced by enforcing checks at least before
committing
to the "master" branch.

Is anyone objecting?

I think that the profile should be defined in the "parent" POM.
Can someone make the necessary additions?

Thanks,
Gilles


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)?

Charles Honton
Since most of us work in an IDE, the "wasted" time of checkstyle for every build is negligible. At my day job, all code is automatically reformatted as part of the build. It's just another step along with PMD, CPD, findbugs, sonar, jacoco, junit, and a few other static analyses. The more we automate, the less we need to remember and the higher the quality of our code.

Chas

> On Aug 8, 2017, at 4:13 PM, Gilles <[hidden email]> wrote:
>
> Hello.
>
>> On Wed, 9 Aug 2017 00:20:00 +0200, Karl-Philipp Richter wrote:
>> Hi,
>>
>>> Am 07.08.2017 um 15:09 schrieb Gilles:
>>> 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".
>> I wouldn't worry about the time consumption of the validation even if
>> it's run by every dev before very compilation since the sum of
>> conversation parts in patch/PR discussion in the form of "LGTM, except
>> for the indentation at line xy" - "OK, I fixed that now" - "Oh, no wait,
>> I forgot the trailing space at line yz" - ... - merged takes an infinite
>> more of time and energy.
>
> I agree, but that is with respect to interaction with someone not used
> to the coding style/rules; what I meant is that when doing one's "own"
> work, one shouldn't have to wait for CheckStyle at every compilation,
> when you know that you'll fix the missing doc _after_ fixing the code.
>
>> Regarding the phase where checkstyle should be run I have some
>> additional thoughts to my initial post:
>>
>>  * If running checkstyle will be enforced the only phase that makes
>> sense is `validate` because you don't won't to build something that's
>> invalid because it's somehow unlogical and a waiste of time if you don't
>> fail the build as early as possible. In order to avoid annoyance for
>> users who aren't used to fix checkstyle errors before being able to
>> build I'd suggest a profile with deactivated checkstyle which allows
>> that rather an putting checkstyle in a separate profile.
>
> IIUC, that would be fine (since it takes care of the above scenario).
>
>>  * Running checkstyle in the site or any other reporting phase is in
>> Maven speak afaik "show what might be wrong with my build given the fact
>> that I consider it passing after compilation, unit and integration tests
>> passed" or "show me some statistics about style issues - 150, wow that's
>> 12 less than last build".
>
> That's what we've done up to now; and the number of errors is supposed
> to be zero before a release.  But I agree that the risk of a lot of work
> for the RM would be reduced by enforcing checks at least before committing
> to the "master" branch.
>
> Is anyone objecting?
>
> I think that the profile should be defined in the "parent" POM.
> Can someone make the necessary additions?
>
> Thanks,
> Gilles
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)?

Gilles Sadowski
On Tue, 8 Aug 2017 18:49:44 -0700, Chas Honton wrote:
> Since most of us work in an IDE, the "wasted" time of checkstyle for
> every build is negligible.

It's not just the wasted time of running the tool (which might
well be negligible), it's the forcing of e.g. documenting a code
that might turn out to be transient on the path to a complete
fix.

> At my day job, all code is automatically
> reformatted as part of the build. It's just another step along with
> PMD, CPD, findbugs, sonar, jacoco, junit, and a few other static
> analyses. The more we automate, the less we need to remember and the
> higher the quality of our code.

That's not always the case: I don't particularly like the "feature"
of IDEs that automatically inserts Javadoc templates:

/**
  *
  *
  * @param a
  * @param b
  * @return int
  */
public int doSomething(int a, int b) {
   // ...
}

Which then often remain useless as actual documentation. ;-)


Regards,
Gilles

>
> Chas
>
>> On Aug 8, 2017, at 4:13 PM, Gilles <[hidden email]>
>> wrote:
>>
>> Hello.
>>
>>> On Wed, 9 Aug 2017 00:20:00 +0200, Karl-Philipp Richter wrote:
>>> Hi,
>>>
>>>> Am 07.08.2017 um 15:09 schrieb Gilles:
>>>> 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".
>>> I wouldn't worry about the time consumption of the validation even
>>> if
>>> it's run by every dev before very compilation since the sum of
>>> conversation parts in patch/PR discussion in the form of "LGTM,
>>> except
>>> for the indentation at line xy" - "OK, I fixed that now" - "Oh, no
>>> wait,
>>> I forgot the trailing space at line yz" - ... - merged takes an
>>> infinite
>>> more of time and energy.
>>
>> I agree, but that is with respect to interaction with someone not
>> used
>> to the coding style/rules; what I meant is that when doing one's
>> "own"
>> work, one shouldn't have to wait for CheckStyle at every
>> compilation,
>> when you know that you'll fix the missing doc _after_ fixing the
>> code.
>>
>>> Regarding the phase where checkstyle should be run I have some
>>> additional thoughts to my initial post:
>>>
>>>  * If running checkstyle will be enforced the only phase that makes
>>> sense is `validate` because you don't won't to build something
>>> that's
>>> invalid because it's somehow unlogical and a waiste of time if you
>>> don't
>>> fail the build as early as possible. In order to avoid annoyance
>>> for
>>> users who aren't used to fix checkstyle errors before being able to
>>> build I'd suggest a profile with deactivated checkstyle which
>>> allows
>>> that rather an putting checkstyle in a separate profile.
>>
>> IIUC, that would be fine (since it takes care of the above
>> scenario).
>>
>>>  * Running checkstyle in the site or any other reporting phase is
>>> in
>>> Maven speak afaik "show what might be wrong with my build given the
>>> fact
>>> that I consider it passing after compilation, unit and integration
>>> tests
>>> passed" or "show me some statistics about style issues - 150, wow
>>> that's
>>> 12 less than last build".
>>
>> That's what we've done up to now; and the number of errors is
>> supposed
>> to be zero before a release.  But I agree that the risk of a lot of
>> work
>> for the RM would be reduced by enforcing checks at least before
>> committing
>> to the "master" branch.
>>
>> Is anyone objecting?
>>
>> I think that the profile should be defined in the "parent" POM.
>> Can someone make the necessary additions?
>>
>> Thanks,
>> Gilles
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)?

sebb-2-2
On 9 August 2017 at 03:57, Gilles <[hidden email]> wrote:
> On Tue, 8 Aug 2017 18:49:44 -0700, Chas Honton wrote:
>>
>> Since most of us work in an IDE, the "wasted" time of checkstyle for
>> every build is negligible.

IDEs vary in how easy it is to set up the checks to agree with the
project settings.
And IDEs vary in the impact on the build time.

>
> It's not just the wasted time of running the tool (which might
> well be negligible), it's the forcing of e.g. documenting a code
> that might turn out to be transient on the path to a complete
> fix.
>
>> At my day job, all code is automatically
>> reformatted as part of the build. It's just another step along with
>> PMD, CPD, findbugs, sonar, jacoco, junit, and a few other static
>> analyses. The more we automate, the less we need to remember and the
>> higher the quality of our code.
>
>
> That's not always the case: I don't particularly like the "feature"
> of IDEs that automatically inserts Javadoc templates:
>
> /**
>  *
>  *
>  * @param a
>  * @param b
>  * @return int
>  */
> public int doSomething(int a, int b) {
>   // ...
> }
>
> Which then often remain useless as actual documentation. ;-)
>
>
> Regards,
> Gilles
>
>
>>
>> Chas
>>
>>> On Aug 8, 2017, at 4:13 PM, Gilles <[hidden email]> wrote:
>>>
>>> Hello.
>>>
>>>> On Wed, 9 Aug 2017 00:20:00 +0200, Karl-Philipp Richter wrote:
>>>> Hi,
>>>>
>>>>> Am 07.08.2017 um 15:09 schrieb Gilles:
>>>>> 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".
>>>>
>>>> I wouldn't worry about the time consumption of the validation even if
>>>> it's run by every dev before very compilation since the sum of
>>>> conversation parts in patch/PR discussion in the form of "LGTM, except
>>>> for the indentation at line xy" - "OK, I fixed that now" - "Oh, no wait,
>>>> I forgot the trailing space at line yz" - ... - merged takes an infinite
>>>> more of time and energy.
>>>
>>>
>>> I agree, but that is with respect to interaction with someone not used
>>> to the coding style/rules; what I meant is that when doing one's "own"
>>> work, one shouldn't have to wait for CheckStyle at every compilation,
>>> when you know that you'll fix the missing doc _after_ fixing the code.
>>>
>>>> Regarding the phase where checkstyle should be run I have some
>>>> additional thoughts to my initial post:
>>>>
>>>>  * If running checkstyle will be enforced the only phase that makes
>>>> sense is `validate` because you don't won't to build something that's
>>>> invalid because it's somehow unlogical and a waiste of time if you don't
>>>> fail the build as early as possible. In order to avoid annoyance for
>>>> users who aren't used to fix checkstyle errors before being able to
>>>> build I'd suggest a profile with deactivated checkstyle which allows
>>>> that rather an putting checkstyle in a separate profile.
>>>
>>>
>>> IIUC, that would be fine (since it takes care of the above scenario).
>>>
>>>>  * Running checkstyle in the site or any other reporting phase is in
>>>> Maven speak afaik "show what might be wrong with my build given the fact
>>>> that I consider it passing after compilation, unit and integration tests
>>>> passed" or "show me some statistics about style issues - 150, wow that's
>>>> 12 less than last build".
>>>
>>>
>>> That's what we've done up to now; and the number of errors is supposed
>>> to be zero before a release.  But I agree that the risk of a lot of work
>>> for the RM would be reduced by enforcing checks at least before
>>> committing
>>> to the "master" branch.
>>>
>>> Is anyone objecting?
>>>
>>> I think that the profile should be defined in the "parent" POM.
>>> Can someone make the necessary additions?
>>>
>>> Thanks,
>>> Gilles
>>>
>>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [ALL] Automated requirements (e.g. CheckStyle)?

Gilles Sadowski
On Wed, 9 Aug 2017 10:56:47 +0100, sebb wrote:

> On 9 August 2017 at 03:57, Gilles <[hidden email]>
> wrote:
>> On Tue, 8 Aug 2017 18:49:44 -0700, Chas Honton wrote:
>>>
>>> Since most of us work in an IDE, the "wasted" time of checkstyle
>>> for
>>> every build is negligible.
>
> IDEs vary in how easy it is to set up the checks to agree with the
> project settings.
> And IDEs vary in the impact on the build time.

Once there is a profile (in "parent"?) and a documented rule
saying "run <some mvn command> and the generated report should
be clean before the PR will be considered for inclusion", why
do we care about IDE x or y?

Gilles

>>
>> It's not just the wasted time of running the tool (which might
>> well be negligible), it's the forcing of e.g. documenting a code
>> that might turn out to be transient on the path to a complete
>> fix.
>>
>>> At my day job, all code is automatically
>>> reformatted as part of the build. It's just another step along with
>>> PMD, CPD, findbugs, sonar, jacoco, junit, and a few other static
>>> analyses. The more we automate, the less we need to remember and
>>> the
>>> higher the quality of our code.
>>
>>
>> That's not always the case: I don't particularly like the "feature"
>> of IDEs that automatically inserts Javadoc templates:
>>
>> /**
>>  *
>>  *
>>  * @param a
>>  * @param b
>>  * @return int
>>  */
>> public int doSomething(int a, int b) {
>>   // ...
>> }
>>
>> Which then often remain useless as actual documentation. ;-)
>>
>>
>> Regards,
>> Gilles
>>
>>
>>>
>>> Chas
>>>
>>>> On Aug 8, 2017, at 4:13 PM, Gilles <[hidden email]>
>>>> wrote:
>>>>
>>>> Hello.
>>>>
>>>>> On Wed, 9 Aug 2017 00:20:00 +0200, Karl-Philipp Richter wrote:
>>>>> Hi,
>>>>>
>>>>>> Am 07.08.2017 um 15:09 schrieb Gilles:
>>>>>> 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".
>>>>>
>>>>> I wouldn't worry about the time consumption of the validation
>>>>> even if
>>>>> it's run by every dev before very compilation since the sum of
>>>>> conversation parts in patch/PR discussion in the form of "LGTM,
>>>>> except
>>>>> for the indentation at line xy" - "OK, I fixed that now" - "Oh,
>>>>> no wait,
>>>>> I forgot the trailing space at line yz" - ... - merged takes an
>>>>> infinite
>>>>> more of time and energy.
>>>>
>>>>
>>>> I agree, but that is with respect to interaction with someone not
>>>> used
>>>> to the coding style/rules; what I meant is that when doing one's
>>>> "own"
>>>> work, one shouldn't have to wait for CheckStyle at every
>>>> compilation,
>>>> when you know that you'll fix the missing doc _after_ fixing the
>>>> code.
>>>>
>>>>> Regarding the phase where checkstyle should be run I have some
>>>>> additional thoughts to my initial post:
>>>>>
>>>>>  * If running checkstyle will be enforced the only phase that
>>>>> makes
>>>>> sense is `validate` because you don't won't to build something
>>>>> that's
>>>>> invalid because it's somehow unlogical and a waiste of time if
>>>>> you don't
>>>>> fail the build as early as possible. In order to avoid annoyance
>>>>> for
>>>>> users who aren't used to fix checkstyle errors before being able
>>>>> to
>>>>> build I'd suggest a profile with deactivated checkstyle which
>>>>> allows
>>>>> that rather an putting checkstyle in a separate profile.
>>>>
>>>>
>>>> IIUC, that would be fine (since it takes care of the above
>>>> scenario).
>>>>
>>>>>  * Running checkstyle in the site or any other reporting phase is
>>>>> in
>>>>> Maven speak afaik "show what might be wrong with my build given
>>>>> the fact
>>>>> that I consider it passing after compilation, unit and
>>>>> integration tests
>>>>> passed" or "show me some statistics about style issues - 150, wow
>>>>> that's
>>>>> 12 less than last build".
>>>>
>>>>
>>>> That's what we've done up to now; and the number of errors is
>>>> supposed
>>>> to be zero before a release.  But I agree that the risk of a lot
>>>> of work
>>>> for the RM would be reduced by enforcing checks at least before
>>>> committing
>>>> to the "master" branch.
>>>>
>>>> Is anyone objecting?
>>>>
>>>> I think that the profile should be defined in the "parent" POM.
>>>> Can someone make the necessary additions?
>>>>
>>>> Thanks,
>>>> Gilles
>>>>
>>>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...