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

Adopt "vers" version range spec #12

Merged
merged 21 commits into from
Nov 30, 2021
Merged

Adopt "vers" version range spec #12

merged 21 commits into from
Nov 30, 2021

Conversation

pombredanne
Copy link
Member

@pombredanne pombredanne commented Nov 15, 2021

This PR add support for the new vers draft specification for version ranges
The primary usage is going to be used in VulnerableCode to support
aboutcode-org/vulnerablecode#525

This a major refactoring of the library

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Also apply minor formatting

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Use namespaces rather than import renaming

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Based on the same code I wrote for Package URL.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
This tests if a version is valid and is best closest to the gentoo code

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Using the join/split idiom is more powerful than just replacing spaces.
It can handle any whitespace.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Major refactoring:

- rename version_range.py to version_constraints.py
- then rename version_specifier.py to version_range.py

The model is now working this way:
- VersionRange is a collection of VersionConstraint with a specific
  Version class implementation to compare versions.
- There is one VersionRange for each version scheme
- A VersionRange can be built from a "vers" string or from a native,
scheme specific syntax.
- A VersionConstraint contains a comparator (such as =) and a Version
  instance
- There are many Version implementations as before
- Not all schemes are implemented, and a scheme is commonly the same
  string as a Package URL type.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated
Comment on lines 42 to 43
- It can parse native version ranges notation into the common "vers" notation
and can return back native version ranges from a "vers".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear.

"parse native version ranges notation into the common "vers" notation" means converting ">1.2.3" to "vers:pypi/>1.2.3"

"can return back native version ranges from a 'vers' " . What is this "native version ranges" ? Is it a string representation of the range or is it an object ?

Copy link
Member Author

Choose a reason for hiding this comment

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

is it a string representation of the range or is it an object ?

This is a string, for instance the one seen in an npm package.json for dependencies or in a setup.py install_requires

>>> assert v24 in VersionConstraint(comparator="<=", version=v24)
>>> assert v24 not in VersionConstraint(comparator="<", version=v24)
"""
if not version:
Copy link
Collaborator

Choose a reason for hiding this comment

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

But for "*" comparator, version is supposed to be None. Hence it will always return false which is opposite of what we're expecting.

Copy link
Member Author

Choose a reason for hiding this comment

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

The version argument here is the version that we want to check. This is not the same as the version attribute of the constraint. You would never pass a constraint as an argument. The if not version: is just a guard against calling the method using None and other falsish values... but to your point this can be confusing and would be caught by the type check done below... so I am removing this line.

if not version:
return False

# TODO: consider if we should accept any type and return False?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is debatable, and I have no clear answer.

Copy link
Member Author

@pombredanne pombredanne Nov 22, 2021

Choose a reason for hiding this comment

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

Raising a ValueError is best for now, removed comment in latest commit

return constraints

@classmethod
def to_constraints_string(cls, constraints):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are doctest possible for this ? Maybe add some examples.

Copy link
Member Author

@pombredanne pombredanne Nov 22, 2021

Choose a reason for hiding this comment

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

Agreed and fixed in latest commit

Return a VersionRange built from an npm "node-semver" range ``string``.
"""
# FIXME: code is entirely duplicated with the GemVersionRange
from semantic_version import NpmSpec
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are the imports lazy ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved these to top level.

Return a VersionRange built from a Rubygem version range ``string``.
"""
# TODO: Gem version semantics are different from semver:
# there can be commonly more than 3 segments
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be added in the README

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

- from_vers() -> from_string() suggested by @sbs2001
- from_native_range() -> from_native()
- to_native_range() -> to_native()

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Based on multiple review feedback from @sbs2001

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
None in VersionConstraint will now raise a ValueError.

Reported-by: @sbs2001
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Suggested by @sbs2001

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Suggested by: @sbs2001

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
VersionConstraint.to_constraints_string is not a trivial function, soe some example help a lot

Suggested by: @sbs2001
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Copy link
Member Author

@sbs2001 Thank you for your detailed review!
I addressed all of your comments.

pombredanne added a commit to Hritik14/vulnerablecode that referenced this pull request Nov 23, 2021
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Create NginxVersionRange.from_native method

Contributed-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Suugested by: @sbs2001
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@pombredanne
Copy link
Member Author

Merging now!

@pombredanne pombredanne merged commit 7a99ab9 into main Nov 30, 2021
@pombredanne pombredanne deleted the adopt-vers-spec branch November 30, 2021 13:46
Hritik14 pushed a commit to Hritik14/vulnerablecode that referenced this pull request Jan 23, 2022
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Hritik14 pushed a commit to Hritik14/vulnerablecode that referenced this pull request Jan 23, 2022
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Hritik14 pushed a commit to Hritik14/vulnerablecode that referenced this pull request Jan 23, 2022
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Hritik14 pushed a commit to Hritik14/vulnerablecode that referenced this pull request Jan 25, 2022
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
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.

2 participants