-
Notifications
You must be signed in to change notification settings - Fork 74
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
Improve NpmSpec parsing #116
base: master
Are you sure you want to change the base?
Conversation
I've left all the commits separate, but I'll squash them once you're happy with the review. |
|
||
else: | ||
blocks = group.split(' ') | ||
blocks = group.split() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now splits across multiple whitespace, but also allows for newlines (so it would allow >0.1.2 \n<3.4.5
)
Would it be worth raising an error at the start if the expression contains newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this is not worth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're referring to "not worth raising an error for newlines", as opposed to allowing multiple spaces between the subclauses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're referring to "not worth raising an error for newlines"
Correct. How many times are there new lines across all the package.json anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wouldn't expect it as an issue, just a slight nitpick of my own code where it's different to the current npm implementation.
I also noticed that |
Reorder OP to check for longer match options first. Co-authored-by: Philippe Ombredanne <[email protected]>
What is missing here? The conflict is only in the changelog and needs to be resolved before merge anyway. |
@pgampe Refer to the linked issue. The maintainer is waiting on clarification from the NPM semver implementers. |
Makes NpmSpec more closely match the
node-semver
implementation in what syntax it accepts as valid.Resolves #115