more typechecking fixes#4755
Conversation
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
Looks mostly good with some questions and some corrections. Needs to be rebased back onto current master which refactored some of the same files and fixed some of the issues addressed here in a better way.
|
|
||
| return data.asdict() | ||
|
|
||
| def _get(self, endpoint: str) -> Optional[Page]: |
There was a problem hiding this comment.
renamed LegacyRepositoy._get() as LegacyRepository._get_page(): so far as I can see it has little in common with its parent class's PyPiRepository._get(), and mypy complains about them not being the same
There was a problem hiding this comment.
I think the correct solution here is going to be refactoring LegacyRepository to not inherit from PypiRepository. I'd suggest doing that in a follow-up PR, and leaving some # type: and # FIXME annotations here in the meantime.
There was a problem hiding this comment.
Even if that's the right long-term fix, get_page() seems to me a sensible name for this method: which gets a Page. Indeed separating this class from the PyPiRepository would leave less reason for their methods to have the same names.
So I'd prefer to leave this change in place: so that we can leave this file mypy-clean - rather than having to whitelist it and allow other errors to slip back in.
(I do not expect to follow up with the change that you propose. Not that I disagree with it; but it's more work than I intend to be taking on here.)
There was a problem hiding this comment.
I think that's sounds logic -- let's go ahead and add a # TODO: annotation above the class as a reminder that this needs to be revisited.
| repositories = [] | ||
|
|
||
| self._lookup: Dict[str, int] = {} | ||
| self._lookup: Dict[Optional[str], int] = {} |
There was a problem hiding this comment.
Not sure why there is an Optional here -- None will all hash to the same type in the dict, and I don't think it should ever end up here. I suspect the error is found elsewhere.
There was a problem hiding this comment.
One place where I have just updated the annotation to match the code but which smells funny to me is in pool.py. Is it really intended that the repository_name indexing self._lookup might be None? I tried asserting that it was not, but lots of testcases failed...
There was a problem hiding this comment.
I'm pretty sure this is unintentional behavior (even if it doesn't cause a bug). For now I'd just whitelist it with an annotation, and circle back to address this on a follow-up PR.
There was a problem hiding this comment.
I'm in two minds about this.
- The advantage of changing the annotation as I have it is that we can then remove this file from the whitelist, and be confident that further typing errors will not happen.
- The advantage of your suggestion is that it reminds us that there is a problem here.
Well, why not both? Suggest that we leave the annotation in place for now - it does, after all, reflect what the code is actually doing - but also add a # FIXME saying "surely there's something wrong here".
(Again, I do not expect to follow-up with any fix for that - this looks like quite a pervasive change).
There was a problem hiding this comment.
I think the compromise here is good. Fair enough!
|
Rebased. I have mostly answered your questions by quoting myself, do say if you'd already seen those comments and still are confused! |
|
|
||
| return data.asdict() | ||
|
|
||
| def _get(self, endpoint: str) -> Optional[Page]: |
There was a problem hiding this comment.
I think the correct solution here is going to be refactoring LegacyRepository to not inherit from PypiRepository. I'd suggest doing that in a follow-up PR, and leaving some # type: and # FIXME annotations here in the meantime.
| repositories = [] | ||
|
|
||
| self._lookup: Dict[str, int] = {} | ||
| self._lookup: Dict[Optional[str], int] = {} |
There was a problem hiding this comment.
I'm pretty sure this is unintentional behavior (even if it doesn't cause a bug). For now I'd just whitelist it with an annotation, and circle back to address this on a follow-up PR.
|
I've added a single FIXME - reasoning, as above, that
You'll say if you're not persuaded that this is good enough! |
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
I'd like to leave one more annotation and then we can land this.
The real purpose of mypy checking (in my mind, and many of the other maintainers I suspect) is to force us to consider the correctness of our code and soundness of design. Any PRs to get a file 'mypy clean' will be touching on correctness as well -- like we saw with the (small but excellent) credentials refactor.
I think that leaving some annotations where we've identified soundness issues thanks to mypy will suffice for now -- and hopefully before too long we can revisit those files and refactor them to be more robust.
|
|
||
| return data.asdict() | ||
|
|
||
| def _get(self, endpoint: str) -> Optional[Page]: |
There was a problem hiding this comment.
I think that's sounds logic -- let's go ahead and add a # TODO: annotation above the class as a reminder that this needs to be revisited.
| repositories = [] | ||
|
|
||
| self._lookup: Dict[str, int] = {} | ||
| self._lookup: Dict[Optional[str], int] = {} |
There was a problem hiding this comment.
I think the compromise here is good. Fair enough!
should the LegacyRepository really inherit from the PyPiRepository?
|
Sounds good, thanks for the thoughtful review |
* new FIXME: are nameless repositories desired/expected? * new TODO: should the LegacyRepository really inherit from the PyPiRepository?
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
2 participants
Pull Request Check List
Resolves: #issue-number-here
Encouraged by seeing #4751 merged - and so fast, thank you! - I took on a few more of the relatively easy mypy warnings.
I have tried to prefer updating the annotations where possible, but this time I have found reasons to make some modest rearrangements to the code:
Config.nameseems to be just broken as there is no_fileattribute. I find no callers, so have removed it.refactored authentication in uploader.py so that mypy can see that the username and password are not
NoneAIUI poetry is dropping support for python2, so I just went with python3 imports of
html.unescapeandurllib.parse.quoterenamed
LegacyRepositoy._get()asLegacyRepository._get_page(): so far as I can see it has little in common with its parent class'sPyPiRepository._get(), and mypy complains about them not being the sameOne place where I have just updated the annotation to match the code but which smells funny to me is in pool.py. Is it really intended that the
repository_nameindexingself._lookupmight beNone? I tried asserting that it was not, but lots of testcases failed...