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

Adding inital types to remote.py #1229

Merged
merged 18 commits into from
May 7, 2021
Merged

Adding inital types to remote.py #1229

merged 18 commits into from
May 7, 2021

Conversation

Yobmod
Copy link
Contributor

@Yobmod Yobmod commented May 3, 2021

Adding types to remote.py:

add_progress()
to_progress_instance()

PushInfo.
init()
old_commit()
remote_ref()

FetchInfo.
refresh()
init()
str()
name()
commit()
_from_line()

  1. A few places the functions can take multiple Reference types including child and parent classes (e.g. Reference, TagReference, RemoteReference, SymbolicReference). In those cases i've used the base class that covers them all (e.g. SymbolicReference). I could change that to a union of all the classes if it would be better.

  2. I copied the compat.typing Literal and Final import logic to types.py, as it would be used in multiple places, not just compat.py. I didn't remove it from compat yet, in case there were bigger plans for it being a module.

  3. Q. PushInfo.init() and FetchInfo() both have an 'old_commit' arg. Should they be the same type (bytes or string rather than a Commit object?) Theres a comment saying should be bytes, but that gives 14 mypy errors, whereas str gives no errors. A leftover from py2?

@Byron
Copy link
Member

Byron commented May 5, 2021

Thanks again for continuing this mammoth of a project of adding types to GitPython! It is much appreciated and I hope people will benefit from it soon.

Do you think this is ready for review? If so, I will see to go through it.

A few places the functions can take multiple Reference types including child and parent classes (e.g. Reference, TagReference, RemoteReference, SymbolicReference). In those cases i've used the base class that covers them all (e.g. SymbolicReference). I could change that to a union of all the classes if it would be better.

Thanks for asking, that won't be necessary. I think GitPython very much had an OO mindset at the time fo writing, so using base classes in general would be preferred where-ever possible.

I copied the compat.typing Literal and Final import logic to types.py, as it would be used in multiple places, not just compat.py. I didn't remove it from compat yet, in case there were bigger plans for it being a module.

The purpose of this module is to avoid duplicating compatibility logic and reduce affairs to a simple import. If that's not possible anymore or not necessary, I wouldn't use that module and it's certainly OK to remove bits and pieces from there if you see a chance. Thinking about it, I hope it's considered 'private' even though it's not marked as such, otherwise it wouldn't even be allowed to remove anything from it really :/.

Q. PushInfo.init() and FetchInfo() both have an 'old_commit' arg. Should they be the same type (bytes or string rather than a Commit object?) Theres a comment saying should be bytes, but that gives 14 mypy errors, whereas str gives no errors. A leftover from py2?

Actually I don't know. I could imagine that old_commit was 20bytes of SHA1 digest, but since this is remote code it won't ever see data in binary but rather strings parsed from the remote's output. It's probably OK to leave it as whatever causes the least trouble and add anything that's missing later as reports come in.

@Yobmod
Copy link
Contributor Author

Yobmod commented May 6, 2021

Yep, I think this can be reviewed then.
I was planning on doing the rest of remote.py once this is merged, to avoid big reviews :)

The only logic changes I think are in PushInfo._from_line() where i separated remote_local_ref into 2 variables to keep track of when it is a string versus a Reference type.

@Byron Byron self-requested a review May 7, 2021 04:08
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.

When looking at the added types and how wide they have to be to fit their usage it becomes painfully clear how easy it was to be quite reckless during development.

Working out all these types (and overloads) is tremendous work and I value it greatly. Everything looks good to me and with CI now running mypi I am confident no issues will arise from merging this one.

Please let me know if you benefit from a patch release and we can try again :).

@Byron Byron merged commit 96f8f17 into gitpython-developers:main May 7, 2021
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.

2 participants