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

add RichPipStreamHandler as allowed handler in repositories/pypi.py #1565

Closed
wants to merge 2 commits into from

Conversation

nicoa
Copy link
Contributor

@nicoa nicoa commented Feb 2, 2022

adressing #1558 .

This needs to tested with actual output. Currently this PR is only adressing the assertion, that now with newest pip version there may be an additional handler - no details inspected about this behaves concretely.

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).

piptools/repositories/pypi.py Outdated Show resolved Hide resolved
@@ -452,7 +461,7 @@ def _setup_logging(self) -> None:
logger = logging.getLogger()
for handler in logger.handlers:
if handler.name == "console": # pragma: no branch
assert isinstance(handler, logging.StreamHandler)
assert isinstance(handler, compatible_handler)

Choose a reason for hiding this comment

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

I think it's going to take more than this. The RichPipStreamHandler sends its output to a rich console, not to a stream.

It looks like this code was added in #949 to fix a problem with displaying the progress bars from pip's verbose output.

But it appears that that fix is no longer needed when the progress bars are rendered using rich. I tested and if I simply remove lines 451-466 and run pip-compile in verbose mode, the progress bars display fine and the command completes successfully with pip 22.

So I would suggest making these lines conditional on whether RichPipStreamHandler was imported above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expected something like this would come up. Thanks for checking! Sounds like a reasonable solution, while I'm not sure if the whole code introduced in #949 can be "reverted / ignored" depending on pip version or we should only skip the assignment of the "proper" stream to the respective handler during execution.

@atugushev would be happy to have your expertise here as you worked on adding this back then in case you have time.

@hugovk
Copy link
Member

hugovk commented Feb 2, 2022

This has been included in #1567, and tests pass.

@wbolster
Copy link
Contributor

wbolster commented Feb 2, 2022

to avoid parallel work and confusion, closing this in favour of #1567

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.

None yet

4 participants