Add --no-implicit-optional to mypy
#10091
Conversation
It must close pypa#10076, to include all the strict flags from Mypy to be used on pypa/pip.
uranusjr
added
the
skip news
|
Hi. I'm having a weird error message. I haven't modified anything for this pull request, but got this: I think it is because |
|
I think you're right, these are issues that mypy detects in strict mode, and they will need fixing before we enable To be clear, I would not assume that all of these are simply incorrect annotations, nor would I necessarily assume that the right fix should be to relax or adjust type annotations to make the checks pass. It's quite possible that mypy is reporting genuine code issues here. (In fact, I sort of hope it is, otherwise we're putting a lot of effort into annotations and it's not giving us much back in terms of better error detection 🙂) |
|
Actually, there are some errors unrelated to annotations (well, I think so 😅 ). For example, look at these fragments: What do you think about them? |
|
Why do you think they are unrelated? Did you check the mypy docs? I found this one, so I suspect the others are from mypy too. |
@pfmoore, I didn't say they were unrelated to mypy, I said they are unrelated to |
|
Sorry, my misunderstanding. It looks like I'm not a good person to ask, I'm at best +0 on stricter typing (it feels to me like we spend more time fixing typing bugs than catching actual code issues, so I feel like we might have hit the point of diminishing returns here). |
Don't worry 😉.
Might be. Anyway, I will keep this PR open if someone wants to comment about this. |
|
It feels like a good point to start though. If we decide we don’t like some typing checks, we can always disable them individually ( |
|
I will first add |
It applies for the Mypy check.
|
If we really need another strictness check, we can simply add them to the mypy |
Does that one not mean that we either need to ask tenacity to change their export list, or change our code to use a dotted name? If we just switch the way we reference the name, I see no great benefit (and two minor disadvantages, in that it's more verbose and it will fail if someone changes it back expecting the two ways to be equivalent). Conversely, if we decide we should ask tenacity to change their code, that will add a whole load of busy-work for both us and them - and unless it turns out that we're actually using an API that they don't support, there's not much benefit. Of course, if you're saying that our response to this message should be that we treat it as saying we're using an unsupported API and we should stop doing so then I guess that is a genuine code problem that the warning has picked up. But would we then rewrite that chunk of code to avoid using the unsupported API? Could we do that? I still feel we've got more pressing things we should be spending our time on. And while I don't object to anyone spending their time however they want to, policy choices like this impact everyone contributing whether they support the check or not. Disclaimer: I only skimmed the mypy description of the message, so I may well have misunderstood what it's saying. Edit: I read the tenacity source. The re-exports in |
|
Seems like we are going to need more time (and more people) to decide how to apply the mypy arguments. I'll convert this to draft. |
|
Thanks. Based on my analysis of the tenacity warning that @uranusjr pointed out, I'm a strong -1 on enabling |
DiddiLeija
changed the title
--strict option to the Mypy checks
|
Is there any other strictness flag we can safely include? Tell me, and I'll add it as soon as possible. |
|
Any other strictness flag to include? We've already added |
pradyunsg
changed the title
|
FWIW, another thing worth doing is removing |
|
@DiddiLeija Could you rebase this PR instead of merging If you don't know what "rebase this PR" means, here's a resource: https://dev.to/matks/what-it-means-to-rebase-a-pull-request-submitted-on-github-5717 :) |
Ok, I'll consider that in the future. Thanks @pradyunsg! |
|
FWIW we can always squash and rebase right before merging, so don't need to sweat too hard on this. |
github-actions
bot
added
the
needs rebase or merge
|
I think #10439 could resolve this. If the "implicit optional" annotations are fixed, then I can merge |
DiddiLeija
changed the title
--no-implicit-optional to mypy
|
What's the next steps here? I haven't really kept up with this whole discussion, and I'm not sure what all we want to do here. |
|
I want to enable |
|
Closing this, since our blocker (#10439) has been closed, so we can't go ahead for now. Maybe I will keep the linked issue open, so maybe we can solve it on the future. |
It must close #10076, to include all the strict flags from Mypy to be used on pypa/pip (they are included by
--strict).