Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork941
Finish initial typing of Index and Submodule#1285
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…on. Breaks typechecking for some reason
Yobmod commentedJul 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I had a question about Traversable.list_traverse(). Should I hard-code list_traverse to be as_edge=False? Or I could change it to return a normal List of tuples when True. |
I think if it’s not a breaking change you should do whatever seems right. If it must be a breaking change a major version bump could be done, but I would love to avoid it. This API suffers from being overly abstract, it worked in a few cases but was kept even when it was out of place :/. |
Yobmod commentedJul 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
ok, ive typed it as though as_edge=False with a type: ignore for now. Otherwise this is ready to merge. Just Refs to go! Re: adding me as a a maintainer. Thats fine by me. |
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.
This is tremendous work, thanks again! And I admit I could only skim through all changes and hope for some pattern matching to kick in.
The changes related to settingas_edge
to false seem to be done for real, but also nothing seems to break in the test suite so I am hopeful it truly doesn't affect anything.
I am looking forward to seeing this work finished, apparently there is just therefs part to go.
@@ -1065,13 +1079,14 @@ def rename(self, new_name): | |||
destination_module_abspath = self._module_abspath(self.repo, self.path, new_name) | |||
source_dir = mod.git_dir | |||
# Let's be sure the submodule name is not so obviously tied to a directory | |||
if destination_module_abspath.startswith(mod.git_dir): | |||
ifstr(destination_module_abspath).startswith(str(mod.git_dir)): |
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 understand that the type checker wanted add these conversions here, but is there a way to do without them?
It's just thatgit_dir
is a directory and its value might not be decodable by the encoding enforced bystr
.
It would be great if you could give it another look and if there is an alternative, you could push that tomain
directly.
But you knoweverything about the types added here :), while having interest to keep the types working for yourself. As I would probably be overwhelmed when trying to fix issues arising from it in future, you are now the major force in keeping this project maintained. |
This fixes three mypy errors by modifying the Diffable.diff,Diffable._process_diff_args, and its IndexFile._process_diff_argsoverride. This change, so far, adds no suppressions, and evenremoves some preexisting suppressions that were ineffective becauseof how they were written (not being on the first line of the def).However, this is not a complete fix of those annotationsthemselves. This is because although the `other` parameter toDiffable.diff and Diffable._process_diff_args does not appearintended to have been as broad as including object in the union hadthe effect of making it -- any union with object as an alternativeis equivalent to object itself -- it should nonetheless be broaderthan the changes here make it.Once that is fixed, it may not be possible to maintain compliancewith the Liskov substitution principle, in which case a suppression,corresponding to one of those that was removed but fixed so it hasan effect, may need to be introduced, since actually fixing the LSPviolation would be a breaking change.Specifically, as seen in797e962 and09053c5 ingitpython-developers#1285, the objectalternative was originally intended not to indicate that any objectis allowed, but instead to allow the NULL_TREE constant, which was(and currently remains) implemented as a direct instance of object.The type of NULL_TREE is not the core problem. It can be fixed toallow a narrowly scoped annotation. One way is by making NULL_TREEan enumeration constant and annotating `Literal[NULL_TREE]`.Rather, the problem is that the IndexFile.diff override really doesnot accept NULL_TREE. It may not be feasible to modify it to acceptit. Even if that is to be done, it should be for runtimecorrectness, and likely have a test case added for it, and may notbe suitable for inclusion alongside these static typing fixes.So when the base class method's annotation is broadened to add sucha literal or equivalent, the override's annotation in the derivedclass may not reasonably be able to change accordingly, as LSPwould dictate.Part of the change here adds a missing os.PathLike[str] alternativeto _process_diff_args. For now I've opted to include both str(already present) and os.PathLike[str] separately, rather thanconsolidating them into PathLike (in GitPython, PathLike isgit.types.PathLike, which is Union[str, os.PathLike[str]]). Thereason is that, while some str arguments may be paths, others maybe options.However, that stylistic choice may not be justified, since it is atodds with some existing uses of PathLike in GitPython to cover bothstr as a path and str as a non-path, including in theimplementation of Diffable.diff itself. So those may beconsolidated in a forthcoming commit.
Uh oh!
There was an error while loading.Please reload this page.