Let remote.push return a PushInfoList

[Sjord]

List-like, so that it's backward compatible. But it has a new method
raise_on_error, that throws an exception if anything failed to push.

Related to #621

This is more of a proposal for this design. Is this actually backward-compatible? Should we check the flags of all PushInfos in raise_on_error? Is this better than a new method push_really_or_else_raise() or push(raise=True)? What if a PushInfo is missing from the result because it totally failed?

[Byron]

Thanks for keeping the ball rolling on solving the matter.

Changing the type of the return value could probably easily be a backward incompatible change unless (probably impossible) care is taken to make the type equivalent even in the presence of instanceof checks.

Hence I think having an entirely new method for it that can do what it wants is the only safe way.

[Sjord]

Changing the type of the return value could probably easily be a backward incompatible change unless (probably impossible) care is taken to make the type equivalent even in the presence of instanceof checks.

Well, the new PushInfoList extends IterableList and list. So isinstance(..., list) would still return true, and PushInfoList can still be iterated over to retrieve PushInfo items. It's true that the return type is different, but I thought that would have limited impact.

@Yobmod, what do you think?

[Byron]

Well, the new PushInfoList extends IterableList and list. So isinstance(..., list) would still return true, and PushInfoList can still be iterated over to retrieve PushInfo items. It's true that the return type is different, but I thought that would have limited impact.

Thanks for the clarification. Could you post an example invocation for me to understand how one would enable exceptions using the return type? My intuition here is that a separate method with different semantics will be similarly opt-in, but inherently more straightforward to use.

[Sjord]

The most straightforward usage is like this:

remote.push().raise_on_error()

The return value of push() is backwards compatible, making it possible to combine the ability to raise exceptions with checking flags.

push_info = remote.push()
if not push_info[0].flags & push_info.NO_MATCH:
    push_info.raise_on_error()

This adds more flexibility for custom error handling, compared to a new method.

A similar pattern is used in requests' raise_for_status, which raises an error when a HTTP request returns an error status code.

[Byron]

Thanks for the clarification! I think my biggest stumbling block was the naming, specifically …on_error() which made me think it's a capability that is turned on to, then, raise if an error occurs. Of course it didn't compute that this would be set on a return value.

Maybe it will help most to rename it to raise_for_error() instead to go with the mainstream, even though my intuition would have been raise_if_error().

Since the the respective field is called exception, I think I would be confused if the actual value is not stored in result.error. Maybe it's better to either not expose this by making the field private, or to rename either method or field for consistency. Since built-in error types are usually called …Error, like IOError, I suppose the field should be named similarly to match.

Once the naming is clear, I think there shouldn't be anything else holding this PR back.

[Byron]

@Sjord Please feel free to mark this PR as ready when ready. If there are problems with types, [AT]Yobmod might be able to provide some clues.

[Byron]

This looks great, thanks a lot.

I think once we can prove that the type system won't break by this change either it's definitely ready to be merged.

Thanks a lot

[Byron]

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())

[Sjord]

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'))

[Byron]

"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.

[Byron]

Thanks for the improvement to the types, much appreciated.

Is it possible to add a test for this type, too, to validate that the new type now passes for the old if used in a typed function like this?

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

f(PushInfoList())

[Sjord]

Is it possible to add a test for this type, too, to validate that the new type now passes for the old if used in a typed function like this?

This is not so easy. I tried the folllowing, but isinstance and issubclass don't work on generic types:

self.assertIsInstance(results, IterableList[PushInfo])

Another possibility would be to let mypy do the testing; mypy would raise an error on your example code if PushInfoList is no longer compatible with IterableList[PushInfo]. But the unittest code is not checked with mypy, so putting it there won't have any effect. I am also not comfortable with putting dead code in git/remote.py just for mypy testing sake.

[Byron]

Another possibility would be to let mypy do the testing; mypy would raise an error on your example code if PushInfoList is no longer compatible with IterableList[PushInfo]. But the unittest code is not checked with mypy, so putting it there won't have any effect. I am also not comfortable with putting dead code in git/remote.py just for mypy testing sake.

Thanks for having given it a shot. I wasn't even aware that the tests aren't yet checked with mypi and can imagine that trying to do that would result in a slew of errors that aren't worth fixing.

If you can tell me something like the code I suggested worked in your local tests, I think we are ready to merge, having tried our best to not break anything.

Thank you

[Sjord]

If you can tell me something like the code I suggested worked in your local tests

Yes, I temporarily put the example you supplied in remote.py, and mypy agrees that a PushInfoList instance is acceptable for a IterableList[PushInfo] parameter.

[Byron]

Thanks a lot for all the work that went into this and for hanging in there :).