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.

Sign up for GitHub

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

Jump to bottom

Add function to invert constraints and ranges #91 #92

Merged
merged 6 commits into from
Nov 23, 2022
Merged

Add function to invert constraints and ranges #91 #92

merged 6 commits into from
Nov 23, 2022

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Nov 9, 2022 •
edited
Loading

Signed-off-by: Tushar Goel tushar.goel.dav@gmail.com

We have invert of every operator:

>= : <
<= : >
!= : =
< : >=
> : <=
= : !=

But we don't have any inversion for star operator currently, what should we do for that ?

@TG1999
Add inverse function to VersionRange
96ac3b8 
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999 TG1999 requested a review from pombredanne November 9, 2022 17:13
@TG1999
Copy link
Contributor Author

TG1999 commented Nov 9, 2022

@pombredanne I made inversion of each comparator here:

INVERTED_COMPARATORS = {
    ">=": "<",
    "<=": ">",
    "!=": "=",
    "<": ">=",
    ">": "<=",
    "=": "!=",
    "*": "^",
    "^": "*",
}

would like to get your opinion on this

pombredanne
pombredanne requested changes Nov 11, 2022
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks... see some comments for your review

src/univers/version_constraint.py Outdated Show resolved Hide resolved
src/univers/version_constraint.py Outdated Show resolved Hide resolved
src/univers/version_constraint.py Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
@TG1999
Address review comments
4d32ece 
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999
Fix doctest
54facef 
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
pombredanne
pombredanne requested changes Nov 16, 2022
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
see my review inline for your consideration.
IMHO the inversion should be first a method on a constraint and then second a range method that use the constraints method.
We would need a few unit tests rather than only data driven tests, and we could get fewer tests by removing duplicated tests.

src/univers/version_range.py Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
src/univers/version_range.py Show resolved Hide resolved
tests/data/inverse-data.json Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
src/univers/version_range.py Outdated Show resolved Hide resolved
@TG1999
Remove duplicate tests
2bec4bf 
Add unit tests for each kind

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999
Edit changelog and remove unwanted test suite
4218340 
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
pombredanne
pombredanne requested changes Nov 21, 2022
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks. Here are a few extra nit pickings for your consideration.

src/univers/version_constraint.py Show resolved Hide resolved
tests/test_version_range.py Outdated
@@ -393,3 +393,20 @@ def test_npm_advisory_version_range_parse(test_case):
string=test_case["npm_native"],
)
assert str(result) == test_case["expected_vers"]


def test_inverse():
Copy link
Member

Choose a reason for hiding this comment

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

You need tests for the lower level constraint.invert e.g., for https://github.com/nexB/univers/pull/92/files#diff-c77d34029ce1991650937489359eef45029f37be4f1f27c21f56e50e64209d9aR138 too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@TG1999
Add tests and docstring for VersionConstraint
637d362 
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@pombredanne pombredanne changed the title Add inverse function to VersionRange #91 Add function to invert constraints and ranges #91 Nov 21, 2022
pombredanne
pombredanne approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

LGTM!
👍

@TG1999 TG1999 merged commit 5d0518c into aboutcode-org:main Nov 23, 2022
@TG1999 TG1999 mentioned this pull request Jan 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pombredanne pombredanne pombredanne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TG1999 @pombredanne