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

Show file tree
Hide file tree
Changes from 7 commits
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
24 changes: 21 additions & 3 deletions git/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,23 @@ def to_progress_instance(progress: Union[Callable[..., Any], RemoteProgress, Non
return progress


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) -> 'PushInfoList':
base = super().__new__(cls, 'push_infos')
return cast(PushInfoList, base)

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

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


class PushInfo(IterableObj, object):
"""
Carries information about the result of a push operation of a single head::
Expand Down Expand Up @@ -774,15 +791,15 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt',

def _get_push_info(self, proc: 'Git.AutoInterrupt',
progress: Union[Callable[..., Any], RemoteProgress, None],
kill_after_timeout: Union[None, float] = None) -> IterableList[PushInfo]:
kill_after_timeout: Union[None, float] = None) -> PushInfoList:
progress = to_progress_instance(progress)

# read progress information from stderr
# we hope stdout can hold all the data, it should ...
# read the lines manually as it will use carriage returns between the messages
# to override the previous one. This is why we read the bytes manually
progress_handler = progress.new_message_handler()
output: IterableList[PushInfo] = IterableList('push_infos')
output: PushInfoList = PushInfoList()

def stdout_handler(line: str) -> None:
try:
Expand All @@ -796,13 +813,14 @@ def stdout_handler(line: str) -> None:
stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or ''
try:
proc.wait(stderr=stderr_text)
except Exception:
except Exception as e:
# This is different than fetch (which fails if there is any std_err
# even if there is an output)
if not output:
raise
elif stderr_text:
log.warning("Error lines received while fetching: %s", stderr_text)
output.error = e

return output

Expand Down
3 changes: 2 additions & 1 deletion test/test_docs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,8 @@ def test_references_and_objects(self, rw_dir):
origin.rename('new_origin')
# push and pull behaves similarly to `git push|pull`
origin.pull()
origin.push()
origin.push() # attempt push, ignore errors
origin.push().raise_if_error() # push and raise error if it fails
# assert not empty_repo.delete_remote(origin).exists() # create and delete remotes
# ![25-test_references_and_objects]

Expand Down
11 changes: 10 additions & 1 deletion test/test_remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
fixture,
GIT_DAEMON_PORT
)
from git.util import rmtree, HIDE_WINDOWS_FREEZE_ERRORS
from git.util import rmtree, HIDE_WINDOWS_FREEZE_ERRORS, IterableList
import os.path as osp


Expand Down Expand Up @@ -128,6 +128,9 @@ def _do_test_fetch_result(self, results, remote):
# END for each info

def _do_test_push_result(self, results, remote):
self.assertIsInstance(results, list)
self.assertIsInstance(results, IterableList)

self.assertGreater(len(results), 0)
self.assertIsInstance(results[0], PushInfo)
for info in results:
Expand All @@ -151,6 +154,12 @@ def _do_test_push_result(self, results, remote):
# END error checking
# END for each info

if any([info.flags & info.ERROR for info in results]):
self.assertRaises(GitCommandError, results.raise_if_error)
else:
# No errors, so this should do nothing
results.raise_if_error()

def _do_test_fetch_info(self, repo):
self.assertRaises(ValueError, FetchInfo._from_line, repo, "nonsense", '')
self.assertRaises(
Expand Down