Skip to content

Let remote.push return a PushInfoList #1360

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

Merged
merged 8 commits into from
Nov 13, 2021
Merged

Let remote.push return a PushInfoList #1360

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
70e4bb8
Let remote.push return a PushInfoList
Sjord Oct 12, 2021
6fc830a
Test that return value of push is a list-like object
Sjord Oct 13, 2021
5c15975
Rename exception to error, raise_on_error to raise_if_error
Sjord Nov 8, 2021
808179e
Test raise_if_error
Sjord Nov 8, 2021
e5fd7b7
Add raise_if_error() to tutorial
Sjord Nov 9, 2021
56a395f
Fix type handing on PushInfoList
Sjord Nov 9, 2021
bc76d4a
Specify type for PushInfoList.error
Sjord Nov 9, 2021
a270deb
Extend IterableList[PushInfo] instead of IterableList
Sjord Nov 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix type handing on PushInfoList
  • Loading branch information
@Sjord
Sjord committed Nov 9, 2021
commit 56a395fb76eac3e5fc629e02a101ed51cc0c8a91
7 changes: 4 additions & 3 deletions git/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,15 @@ def to_progress_instance(progress: Union[Callable[..., Any], RemoteProgress, Non


class PushInfoList(IterableList):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strange thing is that I would expect IterableList[PushInfo] to be the base class, or at least something in this type should indicate that PushInfos are iterated over.

That way, PushInfoList would be fitting in place of IterableList[PushInfo], which might even be a test to add to assure the type system stays compatible, too.

Something like…

def f(l: IterableList[PushInfo]):
    pass

f(PushInfoList())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I had trouble getting this to work with mypy.

The list with pushinfos was created like this:

output: IterableList[PushInfo] = IterableList('push_infos')

I am not sure what the push_infos argument is for, but I tried moving it into PushInfoList:

class PushInfoList(IterableList[PushInfo]):
    def __new__(cls) -> 'PushInfoList':
        return super().__new__(cls, 'push_infos')

    def __init__(self) -> None:
        super().__init__('push_infos')
        self.error: Optional[Exception] = None

However, mypy doesn't agree with this. The types don't match up in __new__. super(IterableList[PushInfo], cls) doesn't work, so now I have a call explicitly to IterableList.__new__. Is that OK?

class PushInfoList(IterableList[PushInfo]):
    def __new__(cls) -> 'PushInfoList':
        return cast(PushInfoList, IterableList.__new__(cls, 'push_infos'))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"push_infos" is supposed to be an id_attribute, which definitely isn't useful here as it is used like this.

Maybe @Yobmod can help us clarify if this .__new__ calling convention to pacify mypi is the expected way, or if there are more idiomatic ways to do this.

From my point of view, it's certainly OK even though I can't tell if a future patch release of mypi is going to stop accepting it.

def __new__(cls) -> 'IterableList[IterableObj]':
return super(IterableList, cls).__new__(cls, 'push_infos')
def __new__(cls) -> 'PushInfoList':
base = super().__new__(cls, 'push_infos')
return cast(PushInfoList, base)

def __init__(self) -> None:
super().__init__('push_infos')
self.error = None

def raise_if_error(self):
def raise_if_error(self) -> None:
"""
Raise an exception if any ref failed to push.
"""
Expand Down