-
Notifications
You must be signed in to change notification settings - Fork 3k
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 --ignore-packages
to pip check
#11159
base: main
Are you sure you want to change the base?
Conversation
Looks great! Thank you for picking this up so quickly! |
This comment was marked as outdated.
This comment was marked as outdated.
@uranusjr what to do?
|
I think this will be very useful so am keen to get it in. Are there any blockers here... other than maintainers time to review? |
Personally a separate option to control ignoring dependecies feels more correct; |
global command option? |
That I’m not sure about, to be honest. A part of me wants to keep things as simple as possible (say just |
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.
Logic looks fine, not sure about the option names and UI design in general so I’m going to leave this open for now.
Hi team, is there anything we can do to move this forward? It's been a year and this is a change some ecosystems would welcome a lot! |
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 a minor tweak, shouldn’t affect functionality (if I got it right)
I pondered on the options names a bit; while I would most like |
Thanks for all the hard work here! 🙏 Not to bikeshed, but if we want a shorter name, maybe we could use That said, ok with the status quo. Having this implemented under any name would be welcome 😀 |
This one is in the 23.3 milestone but CI is red. |
@sbidoul updated |
should_ignore: Optional[Callable[[str], bool]] = None, | ||
should_ignore_dependencies: Optional[Callable[[str], bool]] = None, |
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.
Would be nice to have a docstring on this function explaining how the two arguments are different
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.
like this?
I have had a quick look at this from a end-user perspective, and what strikes me is that the help texts for these two new options are not really helpful to me. Is it correct that Regarding Finally, a little detail: since |
Yes.
For that you should use --recursive-ignore to specify that package could be someone's dependency. There is a test to demonstrate this behaviour. In #11157 it should be use like |
Ok, then this goes against the intuition I get from reading the option name.
But if a package is detected as missing then it is because it is a dependency of another package, otherwise how would pip check know it is missing? So why is I don't know maybe I'm having a bad day but this does not click. At least a better help text is needed, IMO. I don't feel comfortable merging this myself before the release tomorrow but I'm happy to defer to other maintainers. |
I agree. There's no urgency here, let's give it time and see if we can come up with a more intuitive UI. I think part of the problem here is that the examples givent in #11157 (ninja and cmake) have no dependencies, so there's no obvious answer to the question of "what if the package you're ignoring has dependencies?" Maybe we should look for a real-world use case where the ignored package does have dependencies, and base the behaviour on that? Also, do we have a good intuition for what should happen if we have packages B and C installed, where B depends on A 1.0, and C depends on A 2.0, and we say Footnotes |
Today, I could have really used this functionality. Instead, I'll just have to remove |
@dhirschfeld if you read my comment above and the discussion leading up to it, you'll see that there's some debate as to what "this functionality" even is. Could you clarify what precisely your situation is, and what particular variant of the proposed feature would have helped you? |
Funnily enough it has to do with I think Since xref: |
Why are you using Regardless, none of this helps us to decide what the right behaviour for this feature should be, so we're still blocked on clarifying that. |
It's a normal thing to do to ensure packages work with There are other cases where the I think the main use-case of this feature is for |
Sure, there are still some open questions. I'll make some time to put down my thoughts so we can hopefully agree on the right API... |
FYI, this would have been useful to ignore |
Closes #11157