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

fix(bazel): fix major/minor semver check between @angular/bazel npm p… #27635

Closed

Conversation

Projects
None yet
5 participants
@gregmagolan
Copy link
Contributor

gregmagolan commented Dec 12, 2018

…ackage version and angular bazel repo version

Was updating rules_typescript with the semver check (bazelbuild/rules_typescript#352) and noticed the logic error in the check where using ~major.minor.patch as the condition means the version being checked must be>= major.minor.patch whereas using major.minor.x as the condition means the version being checked must only match on major and minor and can have any patch version.

@gregmagolan gregmagolan requested a review from kyliau Dec 12, 2018

@googlebot googlebot added the cla: yes label Dec 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 12, 2018

@kyliau
Copy link
Member

kyliau left a comment

thanks for fixing this! Sorry I wasn't aware of the logic error.
Since we are already using the semver package here do you think it'd be clearer if
it's written as ${major(v)}.${minor(v)}.x instead?
major() and minor() are APIs in the semver package.
https://github.com/npm/node-semver#readme

@gregmagolan gregmagolan force-pushed the gregmagolan:angular-bazel-semver-fix branch from b9dc17f to f38cb7b Dec 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 12, 2018

@kyliau

kyliau approved these changes Dec 12, 2018

@gregmagolan gregmagolan force-pushed the gregmagolan:angular-bazel-semver-fix branch from f38cb7b to 1fba702 Dec 12, 2018

@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

gregmagolan commented Dec 12, 2018

I didn't notice either when reviewing. Only noticed empirically when updating rules_typescript.
Thanks for the suggestion. Updated to use ${major(v)}.${minor(v)}.x. Much cleaner.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 12, 2018

@gregmagolan gregmagolan force-pushed the gregmagolan:angular-bazel-semver-fix branch from 1fba702 to 4339859 Dec 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 12, 2018

fix(bazel): fix major/minor semver check between @angular/bazel npm p…
…ackager version and angular bazel repo version

@gregmagolan gregmagolan force-pushed the gregmagolan:angular-bazel-semver-fix branch from 4339859 to 98613a6 Dec 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 12, 2018

@kyliau kyliau added the comp: bazel label Dec 12, 2018

@ngbot ngbot bot added this to the needsTriage milestone Dec 12, 2018

mhevery added a commit that referenced this pull request Dec 13, 2018

fix(bazel): fix major/minor semver check between @angular/bazel npm p…
…ackager version and angular bazel repo version (#27635)

PR Close #27635

@mhevery mhevery closed this in 1cc08b4 Dec 13, 2018

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.