-
-
Notifications
You must be signed in to change notification settings - Fork 950
Description
RemoteProgress._parse_progress_line
is conceptually a "protected" method: code outside GitPython may subclass RemoteProgress
and override it with another implementation (which may also reasonably call the base class implementation).
The method is conceptually void as implemented in the base class, always implicitly returning None
, and it is annotated accordingly:
Line 582 in afa5754
def _parse_progress_line(self, line: AnyStr) -> None: |
However, while it would not ordinarily be useful to do so, it is--or has been--also intended that it be overridable to return a non-None
value. This is mostly for compatibility with existing code that may have done so in the past, but it may be that reasonable cases for doing so in new code can still arise. This is reflected in code of GitPython:
Lines 685 to 697 in afa5754
def new_message_handler(self) -> Callable[[str], None]: | |
""" | |
:return: | |
A progress handler suitable for handle_process_output(), passing lines on to | |
this Progress handler in a suitable format | |
""" | |
def handler(line: AnyStr) -> None: | |
return self._parse_progress_line(line.rstrip()) | |
# END handler | |
return handler |
And in the subclass of RemoteProgress
introduced in the test suite, which overrides the method:
Lines 54 to 59 in afa5754
def _parse_progress_line(self, line): | |
# We may remove the line later if it is dropped. | |
# Keep it for debugging. | |
self._seen_lines.append(line) | |
rval = super()._parse_progress_line(line) | |
return rval |
But any override that returns a non-None
value is currently inconsistent with the return type annotation on the base class, which is written as -> None
. For Liskov substitutability, an overridden method's return type must be the same or a subclass of the base method's return type, and if a subclass has type annotations, and the return type is specified as a non-Any
type, type checkers such as mypy
and pyright
will report a non-None
return type as a violation.
If possible, the return type in RemoteProgress
should be broadened, but in a way that does not unduly encourage subclasses to return non-None
. See comments on #1788, starting at #1788 (comment), for a discussion and analysis of this. This issue, and the above recommendation, is based on the findings and discussion there.