Skip to content

reformat and refactor#116

Open
eric-milles wants to merge 1 commit intoapache:masterfrom
eric-milles:IVY-1410-redo
Open

reformat and refactor#116
eric-milles wants to merge 1 commit intoapache:masterfrom
eric-milles:IVY-1410-redo

Conversation

@eric-milles
Copy link
Member

restore and update edits from a176a92

part of IVY-1410 and prep for IVY-1656

@bodewig
Copy link
Member

bodewig commented Feb 14, 2026

a few comments from the outside: Personally I'm very hesitant when it comes to "reformat" PRs as often they contain more than that.

In part they may just reflect differences in matters of taste and you tend to get hung up on discussions about naming (like when you rename expectedLine to expectLine in https://github.com/apache/ant-ivy/pull/116/changes?w=1#diff-00b84fcbee9a62d669a81fa0ec8262b6e8d8a3a50ec5f2b96166f74dd9f586b3R143 which I'd disagree with - directly after renaming mergeLine to mergedLine that I'd agree with :-) .

Or they introduce changes beyond that, like this PR seems to be adding tests in https://github.com/apache/ant-ivy/pull/116/changes?w=1#diff-00b84fcbee9a62d669a81fa0ec8262b6e8d8a3a50ec5f2b96166f74dd9f586b3R492 - not that I'd opposed to more tests.

I wonder whether there is a way to solve the formatting issues separately like agreeing on an .editorconfig or something similar.

WRT the changes to the OSGi versions in MANIFEST.MF. Maybe the file should be generated during the build process rather than having to remember to update it?

@eric-milles
Copy link
Member Author

eric-milles commented Feb 15, 2026

The general idea here is that the project had some auto-format applied some time ago that makes it difficult for the human reader/writer. There are some small structural changes in IvyDeliver and IvySettings that provide a nice entry point for a planned change.

Changes to tests shouldn't really require such scrutiny IMO since they are self-testing. expectLine and mergedLine are as such so the names are the same length. I tried to ensure that the tests that were there and relevant to IVY-1410 and IVY-1656 tested conditions carefully -- that is, there are now checks that demonstrate a difference in behavior when I add my fix.

As for the manifest, it should just say 2.6.0 across the board. But I am trying to add changes little-by-little at this point.

@bodewig
Copy link
Member

bodewig commented Feb 15, 2026

Changes to tests shouldn't really require such scrutiny IMO

Let's say we've had pretty bad experience with big commits that claimed to do thing A and also did B - which caused subtle bugs. I'm not going to do with "not much scrutiny" anymore. Call it a personal weakness :-)

expectLine and mergedLine are as such so the names are the same length.

This is not anything I would have ever thought about. And it is an example for "differences in taste". In the end we'll end up arguing about the color of the bike shed longer than about what you really wanted to change.

As for the manifest, it should just say 2.6.0 across the board.

Unless the next release is 2.5.4 ... That's why I believe creating it in an automated way based on the version being built would be better - as a completely separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants