Use property decorator to support typing by Andrej730 · Pull Request #2015 · gitpython-developers/GitPython

Conversation

@Andrej730

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

Byron

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

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

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

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

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.