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: preserve pre-release and build parts of a version on coerce (#592) #671

Merged
merged 12 commits into from
Jan 31, 2024

Conversation

madtisa
Copy link
Contributor

@madtisa madtisa commented Dec 21, 2023

What / Why

Introduces the new coerce option includePrerelease, if set, allowing to preserve pre-release and build parts of a version.

References

Fixes #592
Fixes #357

…m#592)

Introduces the new coerce option `full`, if set, allowing to preserve
pre-release and build parts of a version.
@madtisa madtisa force-pushed the f/support-coercing-prerelease branch from 7c9a156 to 86535f0 Compare December 22, 2023 00:07
@wraithgar
Copy link
Member

Should this be using includePrerelease instead?

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

Should this be using includePrerelease instead?

Isn't that option have different meaning: to compare without taking prerelease versions into account? I can change it, if it won't cause confusion

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

It also includes build part

@wraithgar
Copy link
Member

No it's the opposite. If true, includePrerelease means to include the prereelase portion when doing comparisons. The default is false.

It seems to make sense to extend this boolean to also include the coercion function. What we want to avoid is adding a new single-use boolean with an extremely generic name like "full".

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

No it's the opposite. If true, includePrerelease means to include the prereelase portion when doing comparisons. The default is false.

It seems to make sense to extend this boolean to also include the coercion function. What we want to avoid is adding a new single-use boolean with an extremely generic name like "full".

Yeah, I meant the opposite (despite what I wrote). I understand the point about name is too generic 👌

functions/coerce.js Outdated Show resolved Hide resolved
internal/re.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

This is looking really good, heading in the right direction. Just some regex cleanup and an erroneous comment change. The actual implementation in functions/coerce.js is really good, much easier to read with the parts broken up like that.

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

It's done, should I squash (rebase) everything into one commit at the end?

@wraithgar
Copy link
Member

It's done, should I squash (rebase) everything into one commit at the end?

You don't have to, I will squash this when I merge it.

test/functions/coerce.js Outdated Show resolved Hide resolved
@wraithgar wraithgar self-assigned this Jan 4, 2024
test/functions/coerce.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Thanks for your patience and responses getting this PR ready. This module is one of the most highly downloaded packages on the entire registry and we try to be careful and purposeful when making changes. We're definitely on the right track here and I think this is something that will be able to land soon.

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

Okay, linted and tested locally, so everything should be alright.

I'm still nervous about option name includePrerelease, since it passed all the way to SemVer instance, and it has different meaning there: https://github.com/madtisa/node-semver/blob/5c8efbcb3c6c125af10746d054faff13e8c33fbd/classes/semver.js#L31C1-L32C59.
I would also elevate this change as breaking due to probability this option had been already used with intent that doesn't include parsing prerelease and build parts of a version

@wraithgar
Copy link
Member

We have been the lack of including includePrerelease in a function as a bug, fixing it is not a breaking change. We can leave this PR open for a bit to see if others agree or disagree.

There is still the one odd assertion in test/functions/coerce.js that asserts two different things joined by an || that seems off to me.

@madtisa
Copy link
Contributor Author

madtisa commented Jan 4, 2024

We have been the lack of including includePrerelease in a function as a bug, fixing it is not a breaking change. We can leave this PR open for a bit to see if others agree or disagree.

There is still the one odd assertion in test/functions/coerce.js that asserts two different things joined by an || that seems off to me.

Addressed this in conversation above: #671 (comment)

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.

coerce drops build and prerelease information [BUG] Filter version with prerelease tags
5 participants