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 — Add initial types to repo/fun.py by Yobmod · Pull Request #1192 · gitpython-developers/GitPython

Conversation

@Yobmod

@Yobmod Yobmod commented Mar 4, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Hi,
This adds types to base.py and fun.py.

  • All annotations are consistant according to both mypy and pyright (but will need refining once whole lib is typed).

  • I've used an alias (TBD, 'to be determined') for Any to show places where I was unsure of the type, but i'm sure it should be more specific than Any.

  • i've avoided changing any runtime code as much as possible, except:

    • i've used Union[str, os._pathlike] to annotate path arguments. Where this showed errors due to use of string-only methods, i've then wrapped the arg in str() within the function. This should mean they are all compatible with pathlib.Paths and pathlib.Purepaths, but that will require tests writing before its official.

    • Adding typeguards: e.g. where git_dir is initialised as None, then assigned later to a string, mypy showed places that needed to be checked that it was actually a string and not still None. This could probably be improved by initializing as "" instead but it touched too much code.

    • changed Except (OSError, IOError) to just Except OSError, as typechecking says it was redundant for python 3.

    • Separate tuple unpacking to seperate lines to provide specific types (unpacked tuples cant be typed yet)

Sorry, something went wrong.

Uh oh!

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

Byron reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@Yobmod

Yobmod commented Mar 4, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Finally fixed all automated issues!

All functions in base.py and fun.py now typed and passes mypy and pyright (and flake8)

All reactions

Sorry, something went wrong.

Uh oh!

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

@Yobmod
Combined commits to add types to base.py and fun.py
85ebfb2
@Byron Byron added this to the v3.1.15 - Bugfixes milestone Mar 5, 2021
Byron
Byron requested changes Mar 5, 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.

Thanks a lot for this tremendous work!

Admittedly I only got to fly through about 2/3 of all changes and think there is a high likelihood of me having missed something.
I will see to finish the rest tomorrow but would hope that another maintainer could also have a look.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread git/repo/base.py
GitCommandWrapperType = Git

def __init__(self, path=None, odbt=GitCmdObjectDB, search_parent_directories=False, expand_vars=True):
def __init__(self, path: Optional[PathLike] = None, odbt: Type[GitCmdObjectDB] = GitCmdObjectDB,

@Byron Byron Mar 5, 2021

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.

There is an actual base class for object databases which is more general.
There are two implementations for it.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Yobmod Yobmod Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

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.

LooseObjectDB?

Sorry, something went wrong.

Uh oh!

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

All reactions

@Byron Byron Mar 17, 2021

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.

I think it's best to 'make up' a type and call it ObjectDB, as LooseObjectDB is too specific and kind of wrong here, too. Honestly I don't really know what I was thinking and this clearly is an example of OOP being gone wrong.

Otherwise you could dig into GitDB and see how this DB is composed of various types which provide a bundle of methods which may or may not be that useful in practice.

I do recommend to not get into types of GitDB and instead make them up (inside of GitPython) based on what's common usage in GitPython instead.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread git/repo/base.py
return Head.create(self, path, commit, force, logmsg)

def delete_head(self, *heads, **kwargs):
def delete_head(self, *heads: HEAD, **kwargs: Any) -> None:

@Byron Byron Mar 5, 2021

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.

I think HEAD is too specific - any symbolic reference can be created that way.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Yobmod Yobmod Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

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.

amended in #1202

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread git/repo/base.py
elif config_level == "global":
return osp.normpath(osp.expanduser("~/.gitconfig"))
elif config_level == "repository":
return osp.normpath(osp.join(self._common_dir or self.git_dir, "config"))

@Byron Byron Mar 5, 2021

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.

It's probably nicer to do it like this:

if not (self._common_dir or self.git_dir):
    raise …
return osp.normpath(osp.join(self._common_dir or self.git_dir, "config"))

That way there is no duplicate path transformation.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Yobmod Yobmod Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

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.

amended in #1202

Sorry, something went wrong.

Uh oh!

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

All reactions
@Byron

Byron commented Mar 12, 2021

Copy link
Copy Markdown
Member

As this PR touches a lot of files I am merging it nonetheless even without the requested changes. These can be contributed separately and in the meantime what's there should already be a considerable improvement.

Thanks again, and I am looking forward to your future contributions :).

Yobmod 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 merged commit 63c31b6 into gitpython-developers:master Mar 12, 2021
@Yobmod

Yobmod commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

Sorry for slow reply, I was sent to work somewhere with no internet (shocking common in the UK!)

I'll make the suggested changes on next PR.
As types are added to more files, I'll have to review all the previous ones each time anyway, to check for new typing incompatibilities that arise.

Byron reacted with thumbs up emoji Byron reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@Yobmod Yobmod mentioned this pull request Mar 16, 2021
Add more types #1202
Merged
@muggenhor muggenhor mentioned this pull request Apr 21, 2021
GitCommand missing stderr #1221
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron requested changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Milestone

v3.1.15 - Bugfixes

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 01402503 — 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.