absolute-version / commit-and-tag-version

Fork of the excellent standard-version. Automate versioning and CHANGELOG generation, with semver.org and conventionalcommits.org :trophy:
ISC License
385 stars 36 forks source link

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

Closed paul-barton closed 7 months ago

paul-barton commented 10 months ago

Adding Maven pom.xml File Support in #33

TimothyJones commented 10 months ago

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).

TimothyJones commented 10 months ago

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 commented 10 months ago

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.

paul-barton commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@paul-barton I've got a PR ready for updating the formatting in https://github.com/absolute-version/commit-and-tag-version/pull/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 commented 9 months ago

https://github.com/absolute-version/commit-and-tag-version/pull/114 seems to have been merged. Perhaps this pull request with maven support can be accepted @paul-barton @TimothyJones ?

TaylorHo commented 7 months ago

Do you happen to have any updates on this?

TimothyJones commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

Great! Thanks all :)