Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use property decorator to support typing #2015

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

Andrej730
Copy link
Contributor

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

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

See example below.

import git

path = ""
repo = git.Repo(str(path), search_parent_directories=True)

# Before this commit.
reveal_type(repo.head.object) # Any
reveal_type(repo.head.commit) # Any
reveal_type(repo.description) # Any

# After this commit.
reveal_type(repo.head.object) # AnyGitObject
reveal_type(repo.head.commit) # Commit
reveal_type(repo.description) # str

Copy link
Member

@Byron Byron 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 commented Mar 19, 2025

Comparing mypy output before and after this change, it looks like this introduces nine mypy 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 currently mypy-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.

@Andrej730
Copy link
Contributor Author

Fixed the failing lint.

I guess some of those new mypy errors caused by getters not returning permissive Any's anymore.
And some caused by reference not being overridden by Union["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
Copy link
Member

Byron commented Mar 20, 2025

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.

@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 Byron merged commit 4bedb05 into gitpython-developers:main Mar 20, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants