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

Add semver2 support to npm #237

Closed
wants to merge 2 commits into from
Closed

Add semver2 support to npm #237

wants to merge 2 commits into from

Conversation

cdunford
Copy link
Contributor

  • An optional third paramater is added to setPackageVersion
    to specifiy the version of semver to use. The default value is 1
    to retain backwards compatibility.

fixes #236

Dunford, Craig added 2 commits October 22, 2018 16:10
- An optional third paramater is added to setPackageVersion
  to specifiy the version of semver to use. The default value is 1
  to retain backwards compatibility.
@cdunford
Copy link
Contributor Author

I'm interested to know thoughts. I'm not terribly happy that in order to use semver2 you must provide all 3 parameters, even though others are optional. Perhaps a better solution would be to support a string or options object as the first parameter. If options are provided, they take precedence, even if a 2nd parameter is passed.

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks for your submission.

Nit: I'd prefer the formatting and typings changes as a separate PR, if possible.

*/
export async function setPackageVersion(packageDirectory?: string, srcDirectory?: string) {
export async function setPackageVersion(packageDirectory?: string, srcDirectory?: string, semVer: 1 | 2 = 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than a parameter, can we follow the pattern we use for NuGet packages, by adding a field to version.json to indicate which semver version to use?

https://github.com/AArnott/Nerdbank.GitVersioning/blob/master/doc/versionJson.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good; I'll look into it. In the meantime, I've opened #238 which just includes the typescript upgrade, typings removal and some other minor cleanup changes.

@AArnott
Copy link
Collaborator

AArnott commented Nov 25, 2018

This PR hasn't been updated in a month. Closing. I'll preparing the change I asked for myself. But I've added questions to the original issue that I need to get answers for.

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.

npm setPackageVersion semver2 support
2 participants