Skip to content

Fix typing issues with delete_head and Remote.add #1346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 24, 2021
Merged

Fix typing issues with delete_head and Remote.add #1346

merged 1 commit into from
Sep 24, 2021

Conversation

rra
Copy link
Contributor

@rra rra commented Sep 20, 2021

delete_head and Head.delete historically accept either Head objects
or a str name of a head. Adjust the typing to match.

Using assignment to make add an alias for create unfortunately
confuses mypy, since it loses track of the fact that it's a
classmethod and starts treating it like a staticmethod. Replace
with a stub wrapper instead.

@rra
Copy link
Contributor Author

rra commented Sep 20, 2021

Ah, hm, I missed the impact on Remote.delete. Trying to think about the best way to handle that.

@rra
Fix typing issues with delete_head and Remote.add
5f4b4db 
delete_head and Head.delete historically accept either Head objects
or a str name of a head.  Adjust the typing to match.  This
unfortunately requires suppressing type warnings in the signature of
RemoteReference.delete, since it inherits from Head but does not
accept str (since it needs access to the richer data of
RemoteReference).

Using assignment to make add an alias for create unfortunately
confuses mypy, since it loses track of the fact that it's a
classmethod and starts treating it like a staticmethod.  Replace
with a stub wrapper instead.
@rra
Copy link
Contributor Author

rra commented Sep 20, 2021

Amended with a possible fix. Alternately, perhaps the intent was not to allow strs for Head.delete and they only worked by accident (although in that case I wasn't sure how to create the necessary Head object to pass in).

@rra
Copy link
Contributor Author

rra commented Sep 20, 2021

FYI, for background, this fixes issues that I saw in https://github.com/lsst-sqre/neophile after updating GitPython, namely:

src/neophile/repository.py:58: error: Argument 1 to "delete_head" of "Repo" has incompatible type "str"; expected "Head"  [arg-type]
src/neophile/repository.py:76: error: Argument 1 to "delete_head" of "Repo" has incompatible type "str"; expected "Head"  [arg-type]
src/neophile/pr.py:312: error: Too few arguments  [call-arg]
src/neophile/pr.py:313: error: Argument 1 has incompatible type "Repo"; expected "Type[Remote]"  [arg-type]
src/neophile/pr.py:313: error: Argument 2 has incompatible type "str"; expected "Repo"  [arg-type]

The last three messages are from calling Remote.add with the code as it existed prior to this patch.

@Byron Byron requested a review from Yobmod September 21, 2021 00:27
Byron
Byron approved these changes Sep 21, 2021
View reviewed changes
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 for this contribution, it's much appreciated. Let's allow @Yobmod to also take a look in case I am missing something.

@Byron Byron added this to the v3.1.25 - Bugfixes milestone Sep 21, 2021
@Yobmod Yobmod merged commit 1746b97 into gitpython-developers:main Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Byron Byron Byron approved these changes

@Yobmod Yobmod Yobmod approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v3.1.25 - Bugfixes
Development

Successfully merging this pull request may close these issues.
3 participants
@rra @Byron @Yobmod