portage.porttree: pass basic mypy checks#1437
Conversation
Tatsh
force-pushed
the
typing-porttree
branch
2 times, most recently
from
d36941a to
69241f7
Compare
Tatsh
changed the title
eli-schwartz
left a comment
eli-schwartz
left a comment
There was a problem hiding this comment.
So overall this PR concerns me.
| if TYPE_CHECKING: | ||
| from portage.dbapi import _AuxKeys | ||
| from portage.dep import _match_slot, Atom, match_from_list, use_reduce | ||
| from portage.package.ebuild.config import config as config_type | ||
| from portage.package.ebuild.fetch import _download_suffix | ||
| from portage.util import ensure_dirs, writemsg | ||
| from portage.util.listdir import listdir | ||
| from portage.versions import _pkg_str, catpkgsplit, catsplit, pkgsplit, ver_regexp | ||
| import portage.data |
There was a problem hiding this comment.
No, sorry, this is terrible for maintenance.
There was a problem hiding this comment.
Then I do not see a way we can have full typing within Portage at this time. We basically have to only do simple typing and get the benefits of that but nothing more.
There was a problem hiding this comment.
There is precedence for it:
Line 24 in 6a2bd85
portage/lib/portage/dep/__init__.py
Line 62 in 6a2bd85
There was a problem hiding this comment.
This is now reduced to only names used in type-hinting and not to fill in missing globals that are lazy-loaded.
There was a problem hiding this comment.
There is precedence for it:
No need to cite cases where, unlike what I called out, it is solely annotations. It does not make your point.
Tatsh
force-pushed
the
typing-porttree
branch
2 times, most recently
from
f8f0727 to
0dbb950
Compare
…]; "" replaces None and 0
eli-schwartz
left a comment
eli-schwartz
left a comment
There was a problem hiding this comment.
I have already reviewed this PR. My previous review still stands. A number of my critiques from the previous review have not been answered except by @Tatsh responding "well, this PR is going to do it anyway" and marking the review comment as "status: Resolved", which makes me think it's not worth commenting any further.
If I review your PR and I raise concerns about the approach you've taken, and you disagree with my concerns, then let the record show that I believe this PR should not now or ever be merged in its current shape -- it is not "resolved", we are in dispute over the design points.
This is a GitHub misfeature, that people with a triage bit on a project can be overridden by the PR author.
A discussion should not be closed unless both parties to the discussion agree that it is closed, or someone clicking the merge button says that it is closed. Closing a discussion at "the wrong time" simply leads people to think that objections are settled when they haven't been, so, not everyone is on the same page as each other about whether the PR is agreeable, and people forget what's been said and is still outstanding.
The only way to do so within the current permission system provided by GitHub is for PR authors to only mark a discussion as closed when one of the following applies:
- they believe they have implemented the review request in the desired manner,
"status: applied" - the reviewer says something like "okay fair enough" or "good point" indicating
"status: withdrawn" - the PR author is a project maintainer and is using personal authority to say
"status: overruled"
The current state of affairs is that several discussions have been closed as "status: overruled" by the PR author, which, again, is confusing.
|
wishes for gerrit |
3 participants
Checked with
mypy lib/portage/dbapi/porttree.py | grep -F porttree.py:. Zero issues.Also included are some other changes to match portage-stubs.
The rest of the changes are done by Black.