GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Adding inital types to remote.py by Yobmod · Pull Request #1229 · gitpython-developers/GitPython

Conversation

@Yobmod

@Yobmod Yobmod commented May 3, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Yobmod added 18 commits February 28, 2021 20:56
@Yobmod
add types to base.py and fun.py
ad4079d
@Yobmod
start add types to util.py
5b0028e
@Yobmod
add types to git.util and git.__init__
a094ac1
@Yobmod
add types to git.db and git.exc
71e28b8
@Yobmod
add types to git.compat and git.diff
2fd9f6e
@Yobmod
add initial types to remote.py
6752fad
@Yobmod
copy sys.version checks for literal and final to git.types
18b75d9
@Yobmod
update type of FetchInfo.refresh() to use Literal
a1fa850
@Yobmod
add types to PushInfo.__init__() .remote_ref() and .old_commit()
c08f592
@Yobmod
Merge branch 'main' into addtypes
f8e7a7d
@Yobmod
make progress types more general
baec2e2
@Yobmod
Merge branch 'addtypes' of https://github.com/Yobmod/GitPython into a…
8611488
…ddtypes
@Yobmod
change a type (Commit) to a forward ref
e37ebaa
@Yobmod
change flags type to int
f97d378
@Yobmod
add overloads to to_progress_instance()
90fefb0
@Yobmod
add types to _from_line()
559ddb3
@Yobmod
second pass of adding types
1b16037
@Yobmod
fix Repo forward ref
96f8f17
@Byron

Byron commented May 5, 2021

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Yobmod

Yobmod commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Byron
Byron self-requested a review May 7, 2021 04:08
Byron
Byron approved these changes May 7, 2021
View reviewed changes

@Byron Byron left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 :).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron
Byron merged commit 96f8f17 into gitpython-developers:main May 7, 2021
@EliahKagan EliahKagan mentioned this pull request Oct 20, 2023
We appear to import the submodule version of gitdb (but never do) #1717
Closed
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Yobmod @Byron
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.