Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(updater): add maven pom.xml file support (#33) #109

Closed

Conversation

paul-barton
Copy link

Adding Maven pom.xml File Support in #33

@TimothyJones
Copy link
Member

Nice work! Thanks so much for this!

Looks like the build is failing due to an older node version - when I get a moment I'll bump the node versions in CI and release a breaking change - then you'll be able to rebase on that and it should work (assuming the tests work, but my experience is that PRs that come with tests and documentation generally pass).

test/core.spec.js Outdated Show resolved Hide resolved
@TimothyJones
Copy link
Member

Ok, I've fixed the build - you'll be able to run the tests after a merge.

However, I also noticed that many of the tests weren't actually running, because of dangling chai-as-promised calls. I fixed this by bringing in recommended lint settings for mocha and chai as promised (which also introduced a lot of merge conflict in the test, sorry).

Now that the async test is running, it's sadly is failing due to:

     AssertionError: expected promise to be rejected with an error matching /Failed to read the version field in y…/ but got 'cb is not a function'

@TimothyJones
Copy link
Member

TimothyJones commented Oct 31, 2023

Since the merge was kind of unpleasant, I've pushed a merge commit to your branch

If you're able to fix up the test failure, I'll get this released.

(Also, a note - due to lack of support in mock-fs, the tests won't work on node 20.5 or above)

Edit: I just saw that I accidentally slipped in formatting changes from my usual editor settings - apologies. Feel free to have another go at the merge if you'd prefer a cleaner one.

test/core.spec.js Outdated Show resolved Hide resolved
@paul-barton
Copy link
Author

paul-barton commented Oct 31, 2023

@TimothyJones Ill take a look later today after work and get everything fixed up. I was thinking about adding a --snapshot option for maven projects, which would append the standard maven -SNAPSHOT to any semantic version. I know this diverges from semantic versioning. What are your thoughts around this. I can include it in a separate merge request if you are ok with this.

@TimothyJones
Copy link
Member

I don't really maven, but as I understand it, snapshots are usually prereleases or something incremental like a nightly CI build (is that right?).

Anyway, 1.2.3-SNAPSHOT is actually still a valid semver prerelease identifier. This means that the existing option --prerelease can work:

 --prerelease SNAPSHOT

I think there's probably an ergonomic argument for having --snapshot as a convenience alias, or perhaps "snapshot": true in the json config for the same thing. But again, I don't really maven, so I'm not sure what's convenient - I'd probably defer to you there - if you think it's useful to have an alias, I'll agree.

Following the fork rationale, I'd be happy to accept a convenience option that made it easier to use semver and conventional commits in a maven world.

Unless of course, I'm missing something, and it's really not a semver?

@TimothyJones
Copy link
Member

TimothyJones commented Oct 31, 2023

I had a quick look at the maven docs - it looks like it's pretty liberal with versions, and -SNAPSHOT just goes on the end?

If that's the case, --prerelease SNAPSHOT won't work, but adding -SNAPSHOT to the end is still valid in semver, at least grammatically -

<valid semver> ::= <version core>
                 | <version core> "-" <pre-release>
                 | <version core> "+" <build>
                 | <version core> "-" <pre-release> "+" <build>
                 
 <pre-release> ::= <dot-separated pre-release identifiers>
 
 <build> ::= <dot-separated build identifiers>

and both the build and pre-release identifiers are allowed to include -. So, a --snapshot option that added the identifier to the end of the version wouldn't break any parsers.

It would change the semantics a bit, like you would parse eg 1.2.3-alpha-SNAPSHOT as one pre-release identifier alpha-SNAPSHOT, and 1.2.3-alpha+20230708-SNAPSHOT as having one bit of build metadata, 20230708-SNAPSHOT. But I don't really think that's a real or likely problem. What do you think?

@TimothyJones
Copy link
Member

A small tangent - if you want more meaningful incremental version numbers, that's the original use case for absolute-version. It works pretty well in tandem with commit-and-tag-version (and with a better understanding of how SNAPSHOT is meant to work in maven, I could probably improve that tool too).

@TimothyJones
Copy link
Member

@paul-barton I've got a PR ready for updating the formatting in #114 . I'll wait for this to be ready first, though (or, alternatively, feel free to base it off that and re-raise. Up to you)

@ixuz
Copy link

ixuz commented Dec 7, 2023

#114 seems to have been merged.
Perhaps this pull request with maven support can be accepted @paul-barton @TimothyJones ?

@TaylorHo
Copy link

Do you happen to have any updates on this?

@TimothyJones
Copy link
Member

This PR doesn't pass the tests at the moment, so I'm not able to merge it.

I haven't heard from the author in several months, so I suspect it is abandoned. I'd be happy to accept another PR that provides the same feature, or if the author returns and fixes up this one, that would be fine too.

@TaylorHo
Copy link

@TimothyJones Okay, I understand!

I have installed the @paul-barton's branch using npm i git+https://github.com/paul-barton/commit-and-tag-version/tree/maven-file-support and it worked fine, so I think that there are not so many fixes to make.

I will make a fork of that branch and try to fix the code that isn't passing the tests, and then submit a new PR ;)

TimothyJones pushed a commit that referenced this pull request Jan 15, 2024
* feat: added maven support

* fix: correct updater for pom.xml files
@TimothyJones
Copy link
Member

Thanks to @TaylorHo , this has now been released as 12.2.0.

Thanks very much to Taylor, and also to the original PR author @paul-barton . Much appreciated!

@paul-barton
Copy link
Author

@TaylorHo and @TimothyJones thank you for getting this merged and for all the help, I had some medical issues occur and wasn't able to get back to this PR. Feeling better now, but Thank you again!

@TaylorHo
Copy link

Welcome back! We're glad you're feeling better, @paul-barton!
You did a great job on this PR. We thank you very much 😁

@ixuz
Copy link

ixuz commented Jan 27, 2024

Great! Thanks all :)

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.

None yet

5 participants