You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@maven.apache.org by Joseph Kessselman <ke...@alum.mit.edu> on 2023/10/14 16:52:09 UTC
Feedback sought
I've just issued a pull request proposing that the Apache Xalan-Java
project cut over from Ant-based to Maven-based build.
This is the first time I'm working with Maven, and I'm *sure* I have
done things that are bad form. If anyone has time and energy to glance
at it and sanity-check that I haven't made too much of a mess, that
would be tremendously appreciated.
https://github.com/apache/xalan-java/pull/101
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
For additional commands, e-mail: users-help@maven.apache.org
Re: Feedback sought
Posted by Greg Chabala <gr...@gmail.com>.
On Sat, Oct 14, 2023 at 9:15 PM Joseph Kesselman <ke...@alum.mit.edu>
wrote:
> Taking out the META-INF: I'll look at that. ... quibbles like editor
> configurations and unnecessary files can be cleaned up in subsequent edits;
> they shouldn't be considered blockers now.
>
Sure. Just an opportunity to remove files and thereby reduce the size of
the changeset. They may mostly be relocations, but GitHub's UI still
struggles under the weight of 1400 changes in a single PR.
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
Taking out the META-INF: I'll look at that.
NOTE that as long as this is basically an acceptable framework to replace the And build, and produces correct code, quibbles like editor configurations and unnecessary files can be cleaned up in subsequent edits; they shouldn't be considered blockers now. "Make it work, make it good, make it great. In that order."
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Greg Chabala <gr...@gmail.com>
Sent: Saturday, October 14, 2023 1:50:16 PM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
That's certainly an ambitious changeset, but it takes courage to do great
work.
I'd generally suggest trying to make smaller steps so it's easier on
reviewers.
For instance:
- Could you make a basic Maven build that delegated most of the work to
Ant with maven-antrun-plugin?
- If so, then you could have separate follow up steps for moving code
into the standard Maven layout, and converting from libraries in source to
Maven retrieved versions.
- Could you convert a smaller artifact first, like the samples directory?
As far as specific advice on your current PR:
- I'd recommend taking this opportunity to remove IDE droppings from
source control entirely. .classpath, .project, .settings/* are all Eclipse
specific and will be regenerated when the project is imported, if needed.
They are needless noise in source control and IntelliJ IDEA users don't
care about them at all.
- Everything in META-INF will be generated by Maven, so none of that
should be in source control either.
Re: Feedback sought
Posted by Daniel Widdis <wi...@gmail.com>.
Spotless is another great option.
[
---
| [spotless/plugin-maven/README.md at main ·
diffplug/spotless](https://github.com/diffplug/spotless/blob/main/plugin-
maven/README.md)[github.com](https://github.com/diffplug/spotless/blob/main/plugin-
maven/README.md)
---
](https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md)
Sent from my iPhone
> On Oct 15, 2023, at 7:16 AM, Joseph Kesselman <ke...@alum.mit.edu> wrote:
>
>
> Hm. Doesn't handle language-semantic formatting (yet?), which in my
> experience is often a bigger deal than spaces vs tabs. Good start, though.
>
> \--
> /_ Joe Kesselman (he/him/his)
> -/ _) My Alexa skill for New Music/New Sounds fans:
> / https://www.amazon.com/dp/B09WJ3H657/
>
> () Plaintext Ribbon Campaign
> /\ Stamp out HTML mail!
> ________________________________
> From: Nils Breunese <ni...@breun.nl>
> Sent: Sunday, October 15, 2023 3:01:28 AM
> To: Maven Users List <us...@maven.apache.org>
> Subject: Re: Feedback sought
>
> Joseph Kesselman <ke...@alum.mit.edu> wrote:
>
>
>
>> Re "IDE droppings"... My experience is that they can actually be useful in
expressing things like preferred code formatting style in
importable/executable form. (I'd rather have a standard cross-editor way if
representing that, but I don't know of one.)
>
>
>
> You could take a look at EditorConfig: https://editorconfig.org/
>
> Nils.
>
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
Hm. Doesn't handle language-semantic formatting (yet?), which in my experience is often a bigger deal than spaces vs tabs. Good start, though.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Nils Breunese <ni...@breun.nl>
Sent: Sunday, October 15, 2023 3:01:28 AM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
Joseph Kesselman <ke...@alum.mit.edu> wrote:
> Re "IDE droppings"... My experience is that they can actually be useful in expressing things like preferred code formatting style in importable/executable form. (I'd rather have a standard cross-editor way if representing that, but I don't know of one.)
You could take a look at EditorConfig: https://editorconfig.org/
Nils.
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
Thanks for the pointer.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Nils Breunese <ni...@breun.nl>
Sent: Sunday, October 15, 2023 3:01:28 AM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
Joseph Kesselman <ke...@alum.mit.edu> wrote:
> Re "IDE droppings"... My experience is that they can actually be useful in expressing things like preferred code formatting style in importable/executable form. (I'd rather have a standard cross-editor way if representing that, but I don't know of one.)
You could take a look at EditorConfig: https://editorconfig.org/
Nils.
Re: Feedback sought
Posted by Nils Breunese <ni...@breun.nl>.
Joseph Kesselman <ke...@alum.mit.edu> wrote:
> Re "IDE droppings"... My experience is that they can actually be useful in expressing things like preferred code formatting style in importable/executable form. (I'd rather have a standard cross-editor way if representing that, but I don't know of one.)
You could take a look at EditorConfig: https://editorconfig.org/
Nils.
Re: Feedback sought
Posted by Mark Derricutt <ma...@talios.com>.
Tangential topic - a while ago there was a thread/discussion about
adopting something like spotless/palantir-java-format for automated
formatting.
Whilst a massive change to the code base, which can (potentially) cause
horrors for open branches/PRs unless rebased with care and also
reformatted - but also quite useful /once/ adopted, as such concerns as
the above largely disappear, and any wonky/horrible formatting that
comes up is often more a smell that maybe the code should be cleaner, or
split up more.
I don't recall any definitive decision being made either way, the topic
just seemed to stalemate (at least as far as I saw).
Palantir/spotless does at least require JDK 11+ I believe so that's a
big blocker, at least as far as the build JDK goes.
Mark (with 2cents)
On 15/10/23 3:10 pm, Joseph Kesselman wrote:
> My experience is that they can actually be useful in expressing things
> like preferred code formatting style in importable/executable form.
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
Re "IDE droppings"... My experience is that they can actually be useful in expressing things like preferred code formatting style in importable/executable form. (I'd rather have a standard cross-editor way if representing that, but I don't know of one.) But I don't care that strongly; if consensus is to drop them, I won't object. Nitpick, but a valid one.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Greg Chabala <gr...@gmail.com>
Sent: Saturday, October 14, 2023 1:50:16 PM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
That's certainly an ambitious changeset, but it takes courage to do great
work.
I'd generally suggest trying to make smaller steps so it's easier on
reviewers.
For instance:
- Could you make a basic Maven build that delegated most of the work to
Ant with maven-antrun-plugin?
- If so, then you could have separate follow up steps for moving code
into the standard Maven layout, and converting from libraries in source to
Maven retrieved versions.
- Could you convert a smaller artifact first, like the samples directory?
As far as specific advice on your current PR:
- I'd recommend taking this opportunity to remove IDE droppings from
source control entirely. .classpath, .project, .settings/* are all Eclipse
specific and will be regenerated when the project is imported, if needed.
They are needless noise in source control and IntelliJ IDEA users don't
care about them at all.
- Everything in META-INF will be generated by Maven, so none of that
should be in source control either.
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
There didn't seem to be a good intermediate point.
Maven wanted things organized differently, and I didn't know Maven well enough to argue with it.
Switching from private binaries to Maven packages was a driving force behind the cut-over, and the need to move from jlec to jflex forced the one significant code change.
The "insignificant" ones in Javadoc were an attempt to get a Maven build with no errors before I found the control for turning off doclint. I could back them out but that doesn't seem a good use of resource at this point.
There really isn't that much actual code change. Most of it is moving things into different subdirectories.
It's a one time cost. There's really less needing review than may appear.
If someone wants to volunteer to take my changeset as the final goal and deliver it incrementally, I have no objection. I'm sure someone who knew Maven better could have done this more incrementally.
But I'm what was available, and this is what I could do. And it's actually pretty clean.
I think.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Greg Chabala <gr...@gmail.com>
Sent: Saturday, October 14, 2023 1:50:16 PM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
That's certainly an ambitious changeset, but it takes courage to do great
work.
I'd generally suggest trying to make smaller steps so it's easier on
reviewers.
For instance:
- Could you make a basic Maven build that delegated most of the work to
Ant with maven-antrun-plugin?
- If so, then you could have separate follow up steps for moving code
into the standard Maven layout, and converting from libraries in source to
Maven retrieved versions.
- Could you convert a smaller artifact first, like the samples directory?
As far as specific advice on your current PR:
- I'd recommend taking this opportunity to remove IDE droppings from
source control entirely. .classpath, .project, .settings/* are all Eclipse
specific and will be regenerated when the project is imported, if needed.
They are needless noise in source control and IntelliJ IDEA users don't
care about them at all.
- Everything in META-INF will be generated by Maven, so none of that
should be in source control either.
Re: Feedback sought
Posted by Greg Chabala <gr...@gmail.com>.
That's certainly an ambitious changeset, but it takes courage to do great
work.
I'd generally suggest trying to make smaller steps so it's easier on
reviewers.
For instance:
- Could you make a basic Maven build that delegated most of the work to
Ant with maven-antrun-plugin?
- If so, then you could have separate follow up steps for moving code
into the standard Maven layout, and converting from libraries in source to
Maven retrieved versions.
- Could you convert a smaller artifact first, like the samples directory?
As far as specific advice on your current PR:
- I'd recommend taking this opportunity to remove IDE droppings from
source control entirely. .classpath, .project, .settings/* are all Eclipse
specific and will be regenerated when the project is imported, if needed.
They are needless noise in source control and IntelliJ IDEA users don't
care about them at all.
- Everything in META-INF will be generated by Maven, so none of that
should be in source control either.
Re: Feedback sought
Posted by Alexander Kriegisch <al...@kriegisch.name>.
I have not even looked at the changeset yet, since last time I tried to
help with the taglet problem in the javadoc plugin. I was just following
the conversation a bit and want to drop a few general comments about
contributions and reviews in the OSS world:
If any of us ends up in the role of a reviewer:
-- Let us remember than contributors often want to solve specific
problems and have way less knowledge than we do, if we know the
project in question or the technical domain better than them.
-- Let us try to encourage them, not dishearten them. Let us
acknowledge and commend their efforts.
-- Let us even accept imperfect contrubutions, help fix them up in a
feature branch and them merge if the changes look acceptable. In
some instances, we can even merge first, if the PR does the right
thing and only cosmetic things are to be improved.
-- Let us abuse reviews as a tool to micro-manage the contributor to
change what we would have done differently, until it looks exactly
like a change we would have done. It is both disheartening and a
waste of resources on both sides.
-- Let us not force them to first and foremost cater to our needs as
reviewers, as if our own time was more precious than heirs, just
because we sit in front of the merge button and can exert power,
blocking or accepting a PR.
I often sit on both sides of the table, being both the maintainer of an
OSS product and often a first-time or inexperienced contributor,
scratching my own itches by contributing, donating my time and effort to
do that. I have experienced many disheartening, a few condescending and
even occasionally cruel reivewers. Let us not end up being those
loathsome people, just because we switch seats.
Disclaimer (and back on topic): I agree with most recommendations about
the advantages of small, incremental commits, removing IDE config and
generated files from projects etc. But the latter two are all
imperfections which can be cleaned up later and were not even introduced
by the change itself.
To Greg Chabala: I did not mean *you* with my comments. :-) You were
friendly and professional in your wording and did nothing to scare
anyone off. You did not say anything wrong either. The trigger for me to
write this message was the recommendation to retrofit the big change
into smaller ones post factum now. Joseph was wrestling with Maven and
some of its plugins for the first time. I think, it would be too much to
expect from him to split the commits, just to make them more palatable
for reviewers. I am sure, he could do it, but is it really worth the
time?
Back once more to us in the role of reviewers: If a change to be
reviewed is comple - which can always happen, because software
development occasionally is a complex busi8ness - the GitHub website is
IMO not an adequeste tool to conduct a review. In this case, any serious
reviewer should take the trouble to actually clone the project, check
out the branch in question and look at it in an IDE where it is easier
to navigate the code, slice and dice the changes and diffs, see what was
unchanged and has only been moved, added or deleted, maybe run a build
and see how the code and project structure really "feels" before/after
the change. There, the reviewer also directly has the chance to commit
on top of the contributor's changes and improve them, guiding the
reviewee by example. This kind of collaborative code review is more
welcoming and less disheartening.
Let us all remember: Today's first-time contributor might be tomorrow's
regular contributor who help to unburden from work we are too busy for
in the future. Win-win! Scaring off a first-time contributor is all but
a guarantee that she will never contribute again and use her time for
more rewarding efforts.
Sorry for getting this off my chest here and now, but I felt I wished to
send that message to the Apache Maven community (and actually many
others in the OSS world), even though the message might have been better
suited for the developers list.
Respectfully yours
--
Alexander Kriegisch
https://scrum-master.de
Joseph Kessselman schrieb am 14.10.2023 23:52 (GMT +07:00):
> I've just issued a pull request proposing that the Apache Xalan-Java
> project cut over from Ant-based to Maven-based build.
>
> This is the first time I'm working with Maven, and I'm *sure* I have
> done things that are bad form. If anyone has time and energy to glance
> at it and sanity-check that I haven't made too much of a mess, that
> would be tremendously appreciated.
>
> https://github.com/apache/xalan-java/pull/101
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
> For additional commands, e-mail: users-help@maven.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
For additional commands, e-mail: users-help@maven.apache.org
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
... And I committed my own typo; it should of course have been "moving forward with", not living. Darned auto-correct; I really need to learn to proofread more carefully when typing on the phone.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Joseph Kesselman <ke...@alum.mit.edu>
Sent: Sunday, October 15, 2023 12:48:38 AM
To: Maven Users List <us...@maven.apache.org>
Subject: Re: Feedback sought
Don't worry, we recognized your intent.
I do appreciate the feedback I'm getting. I asked for it, and hopefully the parts that don't make sense to apply right now will be useful in the future.
But, yes, let's focus on whether what I've done is worth living forward with rather than the details of how we got here. I realize that this large a changeset is a pain to review, and I wouldn't normally do it this way. But it isn't as much actual change as it appears to be, and I *think* it's stable enough to seriously consider cutting over to or I wouldn't have issued the PR.
I'd love to see the maven build tested on a few more systems, too. I've run it on Fedora (under WSL) and Windows 10; it would be good to have someone sanity-check it on a Mac.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Alexander Kriegisch <al...@kriegisch.name>
Sent: Saturday, October 14, 2023 11:40:52 PM
To: users@maven.apache.org <us...@maven.apache.org>
Subject: Re: Feedback sought
Alexander Kriegisch schrieb am 15.10.2023 10:36 (GMT +07:00):
> -- Let us abuse reviews as a tool to micro-manage the contributor to
^^^^^^^^^^^^
OMG, of course I meant "let us NOT abuse".
> change what we would have done differently, until it looks exactly
> like a change we would have done. It is both disheartening and a
> waste of resources on both sides.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
For additional commands, e-mail: users-help@maven.apache.org
Re: Feedback sought
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
Don't worry, we recognized your intent.
I do appreciate the feedback I'm getting. I asked for it, and hopefully the parts that don't make sense to apply right now will be useful in the future.
But, yes, let's focus on whether what I've done is worth living forward with rather than the details of how we got here. I realize that this large a changeset is a pain to review, and I wouldn't normally do it this way. But it isn't as much actual change as it appears to be, and I *think* it's stable enough to seriously consider cutting over to or I wouldn't have issued the PR.
I'd love to see the maven build tested on a few more systems, too. I've run it on Fedora (under WSL) and Windows 10; it would be good to have someone sanity-check it on a Mac.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Alexander Kriegisch <al...@kriegisch.name>
Sent: Saturday, October 14, 2023 11:40:52 PM
To: users@maven.apache.org <us...@maven.apache.org>
Subject: Re: Feedback sought
Alexander Kriegisch schrieb am 15.10.2023 10:36 (GMT +07:00):
> -- Let us abuse reviews as a tool to micro-manage the contributor to
^^^^^^^^^^^^
OMG, of course I meant "let us NOT abuse".
> change what we would have done differently, until it looks exactly
> like a change we would have done. It is both disheartening and a
> waste of resources on both sides.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
For additional commands, e-mail: users-help@maven.apache.org
Re: Feedback sought
Posted by Alexander Kriegisch <al...@kriegisch.name>.
Alexander Kriegisch schrieb am 15.10.2023 10:36 (GMT +07:00):
> -- Let us abuse reviews as a tool to micro-manage the contributor to
^^^^^^^^^^^^
OMG, of course I meant "let us NOT abuse".
> change what we would have done differently, until it looks exactly
> like a change we would have done. It is both disheartening and a
> waste of resources on both sides.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@maven.apache.org
For additional commands, e-mail: users-help@maven.apache.org