Re: Feedback sought
Spotless is another great option. spotless/plugin-maven/README.md at main · diffplug/spotlessgithub.comSent from my iPhoneOn Oct 15, 2023, at 7:16 AM, Joseph Kesselman 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 Sent: Sunday, October 15, 2023 3:01:28 AMTo: Maven Users List Subject: Re: Feedback soughtJoseph Kesselman 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
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 Sent: Sunday, October 15, 2023 3:01:28 AM To: Maven Users List Subject: Re: Feedback sought Joseph Kesselman 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
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 Sent: Sunday, October 15, 2023 3:01:28 AM To: Maven Users List Subject: Re: Feedback sought Joseph Kesselman 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
Joseph Kesselman 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
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
... 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 Sent: Sunday, October 15, 2023 12:48:38 AM To: Maven Users List 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 Sent: Saturday, October 14, 2023 11:40:52 PM To: users@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-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org
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 Sent: Saturday, October 14, 2023 11:40:52 PM To: users@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-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org
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-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org
Re: Feedback sought
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-unsubscr...@maven.apache.org > For additional commands, e-mail: users-h...@maven.apache.org > > - To unsubscribe, e-mail: users-unsubscr...@maven.apache.org For additional commands,
Re: Feedback sought
On Sat, Oct 14, 2023 at 9:15 PM Joseph Kesselman 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
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 Sent: Saturday, October 14, 2023 1:50:16 PM To: Maven Users List 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
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 Sent: Saturday, October 14, 2023 1:50:16 PM To: Maven Users List 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
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 Sent: Saturday, October 14, 2023 1:50:16 PM To: Maven Users List 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
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.
