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

Use property decorator to support typing#2015

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 1 commit intogitpython-developers:mainfromAndrej730:main
Mar 20, 2025

Conversation

Andrej730
Copy link
Contributor

It seems this type of properties are not supported by type checkers (mypy, pyright) and usingproperty decorator fixes the issue.

commit=property(_get_commit,set_commit,# type: ignore[arg-type]doc="Query or set commits directly",    )

See example below.

importgitpath=""repo=git.Repo(str(path),search_parent_directories=True)# Before this commit.reveal_type(repo.head.object)# Anyreveal_type(repo.head.commit)# Anyreveal_type(repo.description)# Any# After this commit.reveal_type(repo.head.object)# AnyGitObjectreveal_type(repo.head.commit)# Commitreveal_type(repo.description)# str

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.

Thanks a lot, that's a nice improvement, making the implementation more idiomatic as well.
Once CI passes this can be merged.

@EliahKagan
Copy link
Member

EliahKagan commentedMar 19, 2025
edited
Loading

Comparingmypy outputbefore andafter this change, it looks like this introduces ninemypy errors. I suspect that may be why the decorator was not used for those items earlier.

That doesn't necessarily mean that this shouldn't be merged. GitPython is not currentlymypy-clean internally.mypy errors deliberately do not currently cause CI to fail. But I wanted to make sure this is known, since it is not obvious to me that it wouldn't also affect type-checking in software that uses GitPython. I recommend only merging this if each of the new errors has been considered and their impact has been evaluated.

Byron reacted with thumbs up emoji

@Andrej730
Copy link
ContributorAuthor

Fixed the failing lint.

I guess some of those new mypy errors caused by getters not returning permissiveAny's anymore.
And some caused byreference not being overridden byUnion["Head", "TagReference", "RemoteReference", "Reference"] anymore and instead it's using the actual return type from it's getter (SymbolicReference). No idea, which one fits better - I'm not that familiar with the codebase.

Byron reacted with rocket emoji

@Byron
Copy link
Member

I have to leave the decision to@EliahKagan as well. Since CI can't catch anything I'd be quite confident that this is OK, and if not somebody might be annoyed enough to help with the fix. Since typing is 'tacked on', maybe there is no way to keep it all working anyway, as tools change, and the language is too dynamic as well.
Disposition merge from my side.

Andrej730 reacted with thumbs up emoji

@EliahKagan
Copy link
Member

I think it is definitely better to merge this than to let it wait indefinitely. There might be an improvement that can be made, but I'm not sure. I also don't know when I would get to looking into it further. I have no objection to this being merged at any time; it can always be revisited.

Byron and Andrej730 reacted with thumbs up emoji

@ByronByron merged commit4bedb05 intogitpython-developers:mainMar 20, 2025
25 checks passed
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@Andrej730@EliahKagan@Byron

[8]ページ先頭

©2009-2025 Movatter.jp