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

Improve NpmSpec parsing #116

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gnattishness
Copy link

Makes NpmSpec more closely match the node-semver implementation in what syntax it accepts as valid.

Resolves #115

@gnattishness
Copy link
Author

I've left all the commits separate, but I'll squash them once you're happy with the review.
Thanks for your time!


else:
blocks = group.split(' ')
blocks = group.split()
Copy link
Author

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?

Copy link
Contributor

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

Copy link
Author

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?

Copy link
Contributor

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?

Copy link
Author

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.

@gnattishness
Copy link
Author

I also noticed that NpmSpec would probably accept v>0.1.2 as valid.
I get that v isn't part of semver, but not sure if >v0.1.2 is preferred or neither.
Happy to add something about this in, if you have any thoughts.

Reorder OP to check for longer match options first.

Co-authored-by: Philippe Ombredanne <[email protected]>
@pgampe
Copy link

pgampe commented Mar 9, 2023

What is missing here? The conflict is only in the changelog and needs to be resolved before merge anyway.

@gnattishness
Copy link
Author

@pgampe Refer to the linked issue. The maintainer is waiting on clarification from the NPM semver implementers.
If you'd like action here, I'd be happy for any progress in pushing for an answer from the NPM folks :)

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.

Incomplete support for NpmSpec, according to node-semver
3 participants