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: allow identifierBase to be false #548

Conversation

lsvalina
Copy link
Contributor

What

Added Disable Prerelease Identifier Base flag for creating prerelease identifiers without appending .0/.1
Improved Prerelease Identifier Base to use it when identifier is not set (1.0.0-{Identifier Base}) and adds description to cli help for -n flag

Why

Version 1.0.0-something is valid Semantic Version and its not necessary to have "build" number for identifiers that have only one/ first build, changes reflect that so if another prerelease is created with same identifier the "build" number will be append according Prerelease Identifier Base argument

References

Fixes #441
Improves #532

@lsvalina lsvalina requested a review from a team as a code owner April 12, 2023 14:17
@lsvalina lsvalina requested a review from nlf April 12, 2023 14:17
@wraithgar
Copy link
Member

A "disable" boolean is often very confusing because it's turning something off when it's true.

We already have a flag to change this number, perhaps supporting false would suffice.

@lsvalina lsvalina force-pushed the feature/add_flag_disable_prerelease_identifier_base branch from 157e260 to fda7e36 Compare April 13, 2023 09:39
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
bin/semver.js Outdated Show resolved Hide resolved
classes/semver.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

A very good start. Just some minor copy and code quality feedback.

@wraithgar wraithgar changed the title feat: add Disable Prerelease Identifier Base flag feat: allow identifierBase to be false Apr 13, 2023
classes/semver.js Outdated Show resolved Hide resolved
classes/semver.js Outdated Show resolved Hide resolved
bin/semver.js Outdated Show resolved Hide resolved
@wraithgar wraithgar self-assigned this Apr 13, 2023
['1.2.3-dev.bar', 'prerelease', '1.2.3-dev', false, 'dev', false],
['1.2.0', 'preminor', '1.3.0-dev', false, 'dev', false],
['1.2.3-1', 'preminor', '1.3.0-dev', false, 'dev', false],
['1.2.3-dev', 'prerelease', '1.2.3-dev.1', false, 'dev', false],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nlf what do you think about this? If they have said "increase the prerelease" but also asked for there not to be a prerelease base, do we default to 1 or should we throw?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my gut reaction is we should throw because they've asked for us to do something that doesn't make sense. going from no number to adding a number when we've been asked to not add numbers feels wrong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's throw in this situation. No more guessing user intent!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to clarify do we want to throw also in:

['1.2.0', 'prerelease', '1.2.1-1', false, '', false],

?

Copy link
Member

@wraithgar wraithgar Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh good point. We sure do! "increase the prerelease with no identifier and no identifierBase" sure sounds impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i updated it to throw on both cases

@wraithgar
Copy link
Member

Can you add the tests from #550 to this PR?

@wraithgar wraithgar merged commit 503a4e5 into npm:main Apr 14, 2023
23 checks passed
@wraithgar
Copy link
Member

Really useful new feature, thanks!

Copy link

@Leena8686 Leena8686 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge approved to allow identifierBase to be false

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.

[BUG] Increase pre* with pre-release identifiers without build
4 participants