On Mon, 7 Aug 2017 14:49:16 +0300, Allon Mureinik wrote:
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.
+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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
