Skip to content

Issue 1261 #1264

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 9 commits into from
Jun 4, 2021
Merged

Issue 1261 #1264

merged 9 commits into from
Jun 4, 2021

Conversation

bytefluxio
Copy link
Contributor

Fixes #1261

I accessed private variables instead of adding getters,
because other parts of the code do the same and I didn't
know if there was a reason for it.
E.g.: remote.py line 409:
(...) RemoteReference._common_path_default (...)
@bytefluxio
Copy link
Contributor Author

I initially wanted to add parameterized, but there was an issue somewhere, which is why I decided to just collect the errors

@Byron
Copy link
Member

Byron commented Jun 3, 2021

Thanks a lot!

With tox -e type you should be able to pacify the type checker. It looks like it's not sure startswith actually is available.

@bytefluxio
Copy link
Contributor Author

I come from a strongly typed language, so this caught me unawares.

since reference.py doesn't do a PathLike check, I removed mine, too.

reference.py:

    def __init__(self, repo, path, check_path=True):
        """Initialize this instance
        :param repo: Our parent repository

        :param path:
            Path relative to the .git/ directory pointing to the ref in question, i.e.
            refs/heads/master
        :param check_path: if False, you can provide any path. Otherwise the path must start with the
            default path prefix of this type."""
        if check_path and not path.startswith(self._common_path_default + '/'):
            raise ValueError("Cannot instantiate %r from path %s" % (self.__class__.__name__, path))
        super(Reference, self).__init__(repo, path)

@bytefluxio
Copy link
Contributor Author

bytefluxio commented Jun 3, 2021

Weird, that my tests ran through without issues on Python 3.9.5 ¯\(ツ)/¯ (Without tux, just ran the unit test in my IDE)

@Byron
Copy link
Member

Byron commented Jun 4, 2021

I come from a strongly typed language, so this caught me unawares.

I stopped trying to understand or follow or write python many years ago, too, and I am blissfully unaware of how the typing really works. Removing types is quite alright here I think and more knowledgable individuals can add them later if they see benefits.

Anyway, with a tiny adjustment it worked as expected. Thanks a lot for your contribution - thanks to you GitPython finally does as it says on the packaging (in this instance) 10 years or so later 😅.

@Byron Byron merged commit 01a96b9 into gitpython-developers:main Jun 4, 2021
@Byron Byron added this to the v3.1.18 - Bugfixes milestone Jun 4, 2021
@bytefluxio
Copy link
Contributor Author

bytefluxio commented Jun 4, 2021

Thanks for the test fix.

I fixed that with "Fixes test to not throw false negative results" 057514e

But threw it away accidentally when I "Reverts auto format introduced with 2dbc2be" 79e24f7

Shame on me

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

repo.tag() only accepts 'ref/tags/tagname' instead of just 'tagname'
2 participants