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

Deprecate pip-compile --resolver=legacy #1724

Merged
merged 6 commits into from Nov 13, 2022

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Nov 11, 2022

Resolves #1659

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added the deprecation Related to deprecations or removals label Nov 11, 2022
@atugushev atugushev mentioned this pull request Nov 11, 2022
@atugushev atugushev changed the title Warn about changing default resolver to backtracking Deprecate legacy resolver Nov 11, 2022
@atugushev atugushev changed the title Deprecate legacy resolver Deprecate pip-compile --resolver=legacy Nov 11, 2022
@atugushev atugushev marked this pull request as draft November 11, 2022 23:04
@atugushev
Copy link
Member Author

Now, tests that check the exact output in stderr are failing due to the deprecation warning. I'll address that in a separate PR.

@webknjaz
Copy link
Member

Maybe try using warnings.warn(FutureDeprecationWarning)? And test it using with pytest.deprecated()?

@atugushev
Copy link
Member Author

Maybe try using warnings.warn(FutureDeprecationWarning)? And test it using with pytest.deprecated()?

@webknjaz if so, what's the motivation?

@atugushev atugushev marked this pull request as ready for review November 12, 2022 20:12
@atugushev atugushev added the resolver Related to dependency resolver label Nov 12, 2022
@@ -378,6 +378,13 @@ def cli(
if isinstance(output_file, LazyFile): # pragma: no cover
ctx.call_on_close(safecall(output_file.close_intelligently))

if resolver_name == "legacy":
log.warning(
Copy link
Member

Choose a reason for hiding this comment

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

Why not using Deprecation warnings from warning module? Somehow I find it better.

Copy link
Member Author

@atugushev atugushev Nov 13, 2022

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/exceptions.html#DeprecationWarning

DeprecationWarning
Base class for warnings about deprecated features when those warnings are intended for other Python developers.

DeprecaetionWarning is intended for developers, but pip-tools prints warning for end users. Also, it is "Ignored by the default warning filters", so it should be FutureWarning or something else.

Pure subjectivity image

vs

image

Personally, I prefer the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I don't mind using the warning module if you feel that's better.

Copy link
Member

Choose a reason for hiding this comment

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

I +1 because it was only an observation. I am ok with both approaches. The real benefit of warnings is that they are very easy to silence from outside the application.

@ssbarnea ssbarnea merged commit b776317 into jazzband:master Nov 13, 2022
@atugushev atugushev deleted the default-resolver-change-warning branch November 13, 2022 06:48
@webknjaz
Copy link
Member

@atugushev the motivation is that it's easier to test. Plus if you actually put the warning in the resolution code, it'd be easier to trace what to remove later.
It could be caught and logged differently in the outer CLI layer, I suppose.
Also, there are third-party libs that make it easier to manage depreciations, they are worth exploring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Related to deprecations or removals resolver Related to dependency resolver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate legacy resolver
3 participants