Hello Tomas and Guillaume,
On 13.10.2022 20:58, Tamás Cservenák wrote:
Agree, it does overhead, just like checkstyle (already present) does, but
trust me, minimal overhead (please try some of the PRs).
I might be overreacting about making it default, cause openhab does also
static code analysis (PMD, spotbugs etc) which in total aggregates to
large amount of time. If formatting is fast, and I believe it will stay
rather fast, then it should work. Interestingly enough openhab doesn't
do formatting by default, but does validation by default which often
forced me to do an extra maven pass just to let build go. It was quite
annoying.
Am unsure about your second point: the error message will point to a line
that, if the user opens it in whatever IDE/editor, will correspond to where
the error happened. Or what do you mean here?
If you are in flow, edit file recall you edited line 50, go to console,
run a build to confirm fix for earlier error. You had fix in line 50 but
you get now error in line 53, cause sources were reformatted, so a blank
line you had after code is actually your code. You end up with what the
... moment cause this line could not be source of an error.
This is especially a case for people who have to actually leave IDE to
get console. Also IDEs sometimes have a lag reloading files from
filesystem which then might get even more confusing.
It is something to be communicated early on so people who do sporadic or
first time contributions are not trapped in such situation.
And finally, in this very case diversity does NOT help. We had since (if
not more than) 10 years of published "almost identical" settings on Maven
site for most popular IDEs, but heh: People tend to download them once and
forget (so lag with latest changes), or does not download and produce PRs
with totally wrong formatting, and so on... and ultimately, those settings
are "almost identical", not "100% same".
I get this and that's really what speaks to me mostly. I am not against
doing formatting step, I am just trying to signal risks which it can
bring for people working on slower machines or coming from outside of
core team.
I don't want to spend my whole morning "setting up and onboarding" a
project (by tweaking IDE, downloading plugins for it, and yada yada yada).
I just want to check out the sources and go with it.
T
On Thu, Oct 13, 2022 at 5:13 PM Łukasz Dywicki <l...@code-house.org> wrote:
Some of findings I have:
- Static code analysis might be slow, reformatting code will add
additional time to the build which might not be desired for developer
experience
- Error messages produced after reformatting might mistake first time
contributors who are not familiar with reformatting step done before
compilation
- I recently found a plugin for IJ:
https://github.com/krasa/EclipseCodeFormatter (allows to unify two
dominant IDEs)
- Verification of code style might be automated via pull requests, if
PRs are enforced. Otherwise core developers must take care of their
habits or commit hooks.
Cheers,
Łukasz
On 13.10.2022 15:27, Guillaume Nodet wrote:
Le jeu. 13 oct. 2022 à 12:50, Olivier Lamy <ol...@apache.org> a écrit :
On Thu, 13 Oct 2022 at 17:47, Tamás Cservenák <ta...@cservenak.net>
wrote:
Howdy,
to not get lost in implementation details, it is really unimportant
(spotless, this or that...)
What IS important is that we agree to implement "IDE agnostic source
formatting", that happens during build
(by maven plugin). This means _everyone_ will end up with 100% the same
result.
The only thing that comes to my mind is "sloppy committer" who does not
build but pushes....
not sure to understand. You mean formatting sources will be bind to a
phase and be executed automatically?
Yes, I meant "automatic" in opposition to "on demand". Sorry if that was
not clear.
Wouldn’t it be better to still have a check as today (checkstyle:check
or the used foo-bar-formatter-plugin:check) which checks the sources
at validate phase.
Then in case of failure, the committer can just run
foo-bar-formatted-plugin:format.
So committer can apply some manual changes as well.
this will detect "sloppy" committer as today by simply failing without
adding another plugin running some scm command during the build.
Well, the point is to avoid the multiple round trips between IDE / shell
to
fix all validation issues. Also, you mentioned earlier that IDEs always
have some discrepancies in their formatters, and that's exactly the
reason
why having a single reference formatter (i.e. that one that is run during
the build) is a good idea imho.
Is there some option to have a profile (activated on CI) that detects
there
are "uncommitted changes" post build?
As that would mean that sourcetree contains sources NOT formatted, and
that
CI build did reformat them...
I'd like such a check for PRs. This would definitely ensure authors have
run the build before committing,
and most importantly, that the files are correctly formatted.
But even without this step mitigation is simple: someone just rebuilds
HEAD
and commits...
Thanks
T
On Thu, Oct 13, 2022 at 8:41 AM Tamás Cservenák <ta...@cservenak.net>
wrote:
Formatting happens during build, so once merged all we need is to
merge
master into PR branches...
T
On Thu, Oct 13, 2022 at 8:40 AM Sylwester Lachiewicz <
slachiew...@gmail.com> wrote:
What about already open PR with some features or bug fixes.
Don't we should review and merge it before reformatting code -
otherwise
we
will have lots of merge conflicts and over time no one will have
energy to
review it?
Sylwester
śr., 12 paź 2022, 18:23 użytkownik Guillaume Nodet <
gno...@apache.org
napisał:
I'd like to propose merging the following PRs:
* https://github.com/apache/maven-shared-resources/pull/1
* https://github.com/apache/maven-site/pull/329
* https://github.com/apache/maven/pull/824
* https://github.com/apache/maven-resolver/pull/147
... and more to come
The idea is to use plugins to automatically reformat the source
code and
sort imports to obey the maven coding style.
The first PR adds the necessary resources to maven-shared-resources
: a
new
header file, as there's a requirement to put the header at the very
beginning for the import sorter plugin to work correctly (else it
considers
the license comment to be part of comment headers and screws the
formatting), and the eclipse xml formatter plugin.
The second PR updates the web site to point to that file which
would be
in
git instead of on the maven web site, and also updates the
instruction
for
IDEA since it has been supporting the eclipse xml config for years
now).
The third and fourth PRs are updates on maven and maven-resolver to
apply
those two plugins.
Those plugins have been used on mvnd and maven-build-cache-extension
already, although they are not using a single shared resource that
would be
added by the first PR. Hence mvnd is still using its previous
coding
style.
Another point is that those plugins are fast and only do not process
files
if they have been already processed and untouched since the last
build. So
from a daily development pov, this is transparent and does not
incur any
additional processing time during the build (or not much really).
Cheers,
Guillaume
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org
