-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support ranged upgrades #394
Support ranged upgrades #394
Conversation
This includes a simple test that upgrading a single package does indeed only upgrade that one package.
The underlying functionality for this was already present within `pip-tool`s, however `pip-compile-multi` wasn't aware of it and thus made assumptions about the nature of the spec argument which turned out to be false. The fix here is to teach `pip-compile-multi` how to interpret these package specs to extract the names for the places it needs them while still passing the full spaces through to `pip-tools` for the actual upgrade. Fixes peterdemin#392
ff78755
to
f8c3e69
Compare
The issue reported in peterdemin#393 turns out to have also been fixed by the range handling fix in the previous commit. This adds a test which verifies that. Fixes peterdemin#393
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.
Thank you so much! The code is of high quality. I left a couple of minor comments.
This also makes the pattern case-insensitive and unicode compatible, matching similar patterns in this project.
This ensures a nicer error message for the user.
expected_root, | ||
# Cope with posix style reference data on Windows | ||
replace_name=str(expected_root).replace(os.path.sep, '/'), | ||
) | ||
actual = _load_tree(actual_root) | ||
|
||
assert actual == expected |
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.
Path separators in the output are platform-specific too. I think it's safe for testing purposes to normalize all \
to /
in the actual
output.
if not replace_name: | ||
replace_name = str(root) | ||
return { | ||
x.relative_to(root).name: x.read_text().replace(replace_name, 'ROOT').replace('\\', '/') |
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 added path separator normalization ☝🏻
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.
Oh, right, I missed the separators outside the root 🤦, 👍
Thank you! |
Released in 2.6.3 |
This introduces a generic mechanism for adding test cases based on input files, then uses that to validate the failure mode reported in #392. Finally this applies a fix which then passes the added test.
Fixes #392
This also turns out to have fixed #393, so I've added a test for that too.
Fixes #393