-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Treat imports under "if TYPE_CHECKING" as weak. #15384
Conversation
It's not an error if they're not available at runtime (hence the TYPE_CHECKING test), and at typecheck time they can be provided by [mypy].extra_type_stubs. [ci skip-rust] [ci skip-build-wheels]
Please weigh in on whether you agree that this is even correct. |
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.
Seems correct to me.
I'm not sure this is correct. At least without a more concrete example. My interpretation of If your concrete example shows we might need something like this, I suggest we consider renaming |
(also I'd call this a bugfix for label purposes) |
I think this is correct. We have two different ways of adding type stubs, based on user feedback:
If we did not have #2, then I agree we should not land this PR. @benjyw can you please update the PR description and comments to explain this nuance? |
Also probably the |
Discussed with @Eric-Arellano offline. They helped alleviate my confusion here. Essentially, you aren't importing the type stub, you are importing the package. However it is just for type hints sake. I am a bit worried that we still have a gap in our dependency resolution. Consider the following:
If I run So I'm not sure I'm still 100% convinced we should do this, but I see more or the use-case. @benjyw can you share the real-world case of this cropping up? |
That is all true. But what is unfortunate is the user might be intending to provide that type hint via |
|
So there's two sides to this coin, and I think it's a matter of "which side do we want to support the most"
Given that case 1 is niche and that the user can specify |
Another framing of what @thejcannon is saying. We are only giving false positives in this case right now:
if TYPE_CHECKING:
from pkg_resources import Requirement
def extract_version(req: Requirement) -> str:
return req.version In all other cases, I think it is correct to error. Even if you are using -- Given the escape hatch with |
What I'm grasping towards, weakly, is the following observation, using your example above: If But, if |
That's what |
Bump on #15334, which will make it more clear what to do when this happens. |
When adopting Pants in a new repo, having to go to every |
I guess this goes back to dependency scopes... #12794 |
@thejcannon and my point is that this should not be the case. Most the time you will have a corresponding implementation to go along with it. If you are using type hints for |
I will close this PR, as it seems to be a wider issue, totally unrelated to TYPE_CHECKING. For example, I'm seeing issues where a test has This is tricky because What would be the best practice in cases like this? Add pytest to the default-python resolve, even though it also has its own resolve? |
FWIW I was originally +1 as well. This is admittedly complex if 4 maintainers' initial reaction was incorrect. To stretch this further. I'd happily approve a PR which adds a test ensuring TYPE_CHECKING imports aren't weak, and we can put a nice comment explaining why. |
Isn't this just the very old problem of having 1 dependencies field? dependencies can mean different things in different contexts, but we cram all those contexts into 1 field, which can be a lie. Those contexts are rules, and some rules will have a different notion of dependencies than others. A rule trying to run pytest will not care about dependencies only needed when typechecking, a rule trying to generate a sphinx doc site will care about a different augmented set of dependencies that may mix in css, say, above and beyond symbols that might be referenced in the docs that requires "normal" dependencies to be involved. |
@jsirois it is. However in the 80% case one field is "good enough" as the deps for type checking are the same for running are the same for testing. However, that leads us here 😅 |
And cramming is deemed good because it makes the 80% somehow alot easier / more ergonomic than it would otherwise be possible to do? |
Interesting discussions here, and I agree this is a sign of a wider issue. John is onto something important as well. I have a real case where I had issues with extras for a dependency. It is a single (util) library, and for better or worse, it holds both runtime and test time utility functions. For my tests, I'd like to include the test utils from that library, but I don't want that extra to be included for my app when packaging it up, then it should only include the extras for the runtime utils. I didn't spend enough time on this particular issue then to really figure out how to solve this, but I feel it is related to this discussion here, as input use case if nothing else. |
Yeah, |
Just adding to this sideband discussion, "built" targets are a candidate for 2 dependency classes: build-time and run-time. |
It's not an error if they're not available at runtime (hence the
TYPE_CHECKING test), and at typecheck time they can be provided by
[mypy].extra_type_stubs.
[ci skip-rust]
[ci skip-build-wheels]