-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Exclude coverage for older Python version checks #4438
Exclude coverage for older Python version checks #4438
Conversation
@@ -20,3 +20,6 @@ exclude_also = | |||
# jaraco/skeleton#97 | |||
@overload | |||
if TYPE_CHECKING: | |||
# Coverage is only run on the latest Python version | |||
# these will never be considered covered and we aim for 100% coverage | |||
if sys\.version_info < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this matches <= (3, 12)
but I think that's enough of an edge case, would require more complex regex and extra book-keeping when bumping the Python version, that it's not worth accounting for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is managed primarily in skeleton and there's already an effort underway to implement coverage aggregation, which would obviate the need for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better since then coverage would be correct in telling you that an environment-specific path was never taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you agree. I'll assume we should close this request, then, and await/pursue the upstream change. If you feel differently, feel free to advocate for including this change and we can re-open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have someone working on a more permanent solution that avoids adding special exclusions per-project and making the code conform to a sub-optimal convention, I'm inclined to say let's just wait for the more permanent solution.
@@ -20,3 +20,6 @@ exclude_also = | |||
# jaraco/skeleton#97 | |||
@overload | |||
if TYPE_CHECKING: | |||
# Coverage is only run on the latest Python version | |||
# these will never be considered covered and we aim for 100% coverage | |||
if sys\.version_info < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is managed primarily in skeleton and there's already an effort underway to implement coverage aggregation, which would obviate the need for this change.
StrPath = Union[str, os.PathLike[str]] # Same as _typeshed.StrPath | ||
else: | ||
if sys.version_info < (3, 9): | ||
StrPath = Union[str, os.PathLike] | ||
else: | ||
StrPath = Union[str, os.PathLike[str]] # Same as _typeshed.StrPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've historically written > (3, 9)
(or the equivalent >= (3, 9)
) because I like to put the canonical, preferred code ahead of the legacy code. I don't feel strongly about it and I feel less strongly about it when the compatibility logic is in a compat
submodule.
My thinking is that if someone is skimming the code, if the legacy code is first, they'll need to read the whole block to get a modern understanding of what's going on, whereas if the preferred code is first, they can stop after reading the first block and just know that the rest is for older Python versions.
This concern illustrates is a case where an unless
syntax would be so nice to have:
do:
# preferred behavior
unless sys.version_info < (3, 9):
# legacy behavior
Unfortunately, that's not a thing in Python, so the reader is forced to choose between the more elegant if clause or the more elegant preferred-first ordering.
Summary of changes
Something I noticed from coverage failing for 1 line in #4436 . The rationale is explained in config comment:
Pull Request Checklist
newsfragments/
. (not public facing)(See documentation for details)