-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Python 3.12.1 start time #284
Conversation
…s the latest version of Black.
* fix/flag potential bugs highlighted by mypy.
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.
Approving since I don't know when I'll be able to circle back, but you should check out the one comment with a "
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.
👍 LGTM -> All these typing & small cleanup changes make sense to me.
text = text.decode("utf-8") | ||
# Compensate for windows' anti-social unicode behavior | ||
if self._ascii_only_output and not self.disable_unidecode: | ||
# Windows doesn't actually want unicode, so we get | ||
# the closest ASCII equivalent | ||
text = text_type(unidecode(text)) | ||
# Since coverage doesn't like us switching out it's stream to run extra |
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 always thought the english language was really off base when it decided it's
was an exception to the apostrophe rules.
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.
Seriously. English is just messed up. I know the rule, and still I mess it up when I'm not thinking about it.
self.stderr_errput = OrderedDict() | ||
def __init__(self, stream, *, colors: Colors | None = None): | ||
self.stdout_output: dict[ProtoTest, Any] = {} | ||
self.stderr_errput: dict[ProtoTest, Any] = {} |
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.
Insertion order is guaranteed to be preserved in dicts since Python 3.7 (and were implemented as such in 3.6). Since we do not use the extended features of OrderedDict, it is better to just use the normal ones.
https://docs.python.org/3/library/stdtypes.html#mapping-types-dict
Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.
for target in targets: | ||
if not list(filter(None, [target in x for x in modules])): | ||
if not list(filter(None, (target in x for x in modules))): |
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.
Note this can probably be replaced with something like:
if not any((target in x for x in modules)):
But I need to fully grasp the original condition first, this can be an optimization for the future PR.
@@ -174,14 +181,14 @@ def write(self, text): | |||
self.coverage_percent = int(percent_str) | |||
self.stream.write(text) | |||
|
|||
def writelines(self, lines): | |||
def writelines(self, lines: Iterable[str]) -> None: |
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.
FYI Iterable[str]
is double edged as typing is concerned because strings are iterable lists of strings, so this typing will not catch if a caller is passing a plain string. This is an open 'bug' in mypy, but still better than having not typing at all.
self.collectedDurations = [] | ||
self.errors = [] | ||
self.expectedFailures = [] | ||
self.failures = [] | ||
self.passing = [] | ||
self.skipped = [] | ||
self.unexpectedSuccesses = [] | ||
self.reinitialize() |
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.
FYI, linters are not too happy when properties are declared outside init, moving the lists declarations to __init__()
helps with that and the list can be made empty again with .clear()
.
if self.start_time: | ||
self.test_time = str(time.time() - self.start_time) | ||
else: | ||
self.test_time = "0.0" |
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 is specifically for skipped tests that do not have a start time but they have stopTest
called on them in python 3.12.1
This fixes the start time issue with python 3.12.1 but we are still incompatible due to other issues.
In an effort to help debug more this adds more type annotation to the existing code and fix some of the bugs uncovered by mypy.