Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
Byron merged 73 commits intogitpython-developers:mainfromYobmod:main
Jul 11, 2021
Merged

Finish initial typing of Index and Submodule#1285

Byron merged 73 commits intogitpython-developers:mainfromYobmod:main
Jul 11, 2021

Conversation

Yobmod
Copy link
Contributor

@YobmodYobmod commentedJul 9, 2021
edited
Loading

  • Finished adding initial types to Index and Submodule modules
  • Gone back to try and improve types in various places, especially git/diff.py objects.tree and Traversable
  • Renamed 'diff' in various places (I think none user facing) to distingish the submodule from the function and the variables.
  • Added some TypeGuards (is_config_level()) and Protocols (Has_Repo, Has_id_attribute)
  • Removed 'out_append = out.append' in a couple of places, just because they prevent getting info on hover in IDEs
  • Made IterableListgetattr return type same as the list was initialised with, , which is not strictly correct but better than Any.

Yobmod added30 commitsJuly 5, 2021 12:31
Yobmod added18 commitsJuly 8, 2021 23:55
@Yobmod
Copy link
ContributorAuthor

Yobmod commentedJul 9, 2021
edited
Loading

I had a question about Traversable.list_traverse().
Currently, it transparently passes any args to self.traverse(), which means it can return a tuple of each node when as_edge=True.
But an IterableList of tuples isn't useful as has noid_attribute to select them with, and each tuple will be half redundant info.

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.

@Byron
Copy link
Member

I had a question about Traversable.list_traverse().
Currently, it transparently passes any args to self.traverse(), which means it can return a tuple of each node when as_edge=True.
But an IterableList of tuples isn't useful as has noid_attribute to select them with, and each tuple will be half redundant info.

Should I hard-code list_traverse to be as_edge=False? Or I could change it to return a normal List of tuples.

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
Copy link
ContributorAuthor

Yobmod commentedJul 9, 2021
edited
Loading

ok, ive typed it as though as_edge=False with a type: ignore for now.
So if people override the default they'll just get wrong type hints.
I'll look at adding overloads in the future, but it would need a lot of them to really be correct.

Otherwise this is ready to merge. Just Refs to go!

Re: adding me as a a maintainer. Thats fine by me.
But I don't really know much about Git, thats why I use Gitpython in the first place, lol

Copy link
Member

@ByronByron left a 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)):
Copy link
Member

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.

@ByronByron merged commit9523033 intogitpython-developers:mainJul 11, 2021
@Byron
Copy link
Member

But I don't really know much about Git, thats why I use Gitpython in the first place, lol

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.

@ByronByron added this to thev3.1.19 - Bugfixes milestoneJul 23, 2021
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestMar 9, 2024
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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

Assignees
No one assigned
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
@Yobmod@Byron

[8]ページ先頭

©2009-2025 Movatter.jp