Skip to content

Have duplicate metadata and invalid req strings honor --warn option#357

Merged
kemzeb merged 7 commits intotox-dev:mainfrom
kemzeb:refactor-warning-logic
Apr 28, 2024
Merged

Have duplicate metadata and invalid req strings honor --warn option#357
kemzeb merged 7 commits intotox-dev:mainfrom
kemzeb:refactor-warning-logic

Conversation

@kemzeb
Copy link
Copy Markdown
Collaborator

@kemzeb kemzeb commented Apr 20, 2024

This resolves #355 by making changes and refactors to the warning logic. It does so by introducing a module-level singleton "WarningPrinter" object and refactors the code in such a way to integrate this object for it to be used.

@kemzeb
Copy link
Copy Markdown
Collaborator Author

kemzeb commented Apr 25, 2024

Still working on this, I'll see if I can finish this and open it for review by the end of today.

More specifically, the duplicate metadata and invalid req string
warnings.
@kemzeb kemzeb force-pushed the refactor-warning-logic branch from 4a4f201 to e2e0d12 Compare April 26, 2024 06:24
@kemzeb
Copy link
Copy Markdown
Collaborator Author

kemzeb commented Apr 26, 2024

Did a redesign and this seems to be a better implementation than the last. Just need to add coverage tests then I'll open this PR for review

@kemzeb
Copy link
Copy Markdown
Collaborator Author

kemzeb commented Apr 26, 2024

I think this is ready to go, going to open this for review

@kemzeb kemzeb marked this pull request as ready for review April 26, 2024 16:31
@kemzeb
Copy link
Copy Markdown
Collaborator Author

kemzeb commented Apr 26, 2024

Here are my design decisions:

  • Decided to keep the warning printing logic in their respective modules since they appear to be tightly coupled together
  • Included a _warning module; did try to add it to render but I encountered dependency cycle errors
  • Implemented a WarningPrinter class, where I also exposed a shared module-level singleton of it
  • Decided that the duplicate metadata warning should not be considered a "fail" warning when --warn fail is passed. My reasoning for this is that Python allows these duplicate distributions to exist and pip actively ignores them.
  • I thought it would be nice for WarningPrinter.print_multi_line() to pass what file to write to to the callback so that we avoid throwing sys.stderr everywhere, but due to an odd long-standing bug with the capsys/capfd fixtures, it won't properly capture sys.stderr if we were to pass it down to the callback.

@kemzeb kemzeb changed the title Have new warnings honor --warn option and refactor warning logic Have duplicate metadata and invalid req strings honor --warn option Apr 26, 2024
@xiacunshun
Copy link
Copy Markdown
Collaborator

xiacunshun commented Apr 28, 2024

Ignore my previous deleted comment, I've reconsidered and the handling of "duplicated" here should be fine.

@xiacunshun
Copy link
Copy Markdown
Collaborator

No questions for me here. But I don't have the approve auth, so maybe need to wait for @gaborbernat to take a further look.

@kemzeb
Copy link
Copy Markdown
Collaborator Author

kemzeb commented Apr 28, 2024

Great, will release 2.19.0 after merging this

@kemzeb kemzeb merged commit 4a5f90a into tox-dev:main Apr 28, 2024
@kemzeb kemzeb deleted the refactor-warning-logic branch April 28, 2024 19:39
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.

Consider --warn option when dealing with duplicate metadata and invalid requirement string warnings

3 participants