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
Use lowercase hex numbers fixes #1692 #1775
Changes from 5 commits
da6743f
a607ada
0758897
d7339e8
ffff687
4aa80bc
831343b
cf07525
84554f6
dd80843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5174,35 +5174,78 @@ def normalize_numeric_literal(leaf: Leaf) -> None: | |
in Python 2 long literals). | ||
""" | ||
text = leaf.value.lower() | ||
if text.startswith(("0o", "0b")): | ||
if is_oct_or_binary(text): | ||
# Leave octal and binary literals alone. | ||
pass | ||
elif text.startswith("0x"): | ||
# Change hex literals to upper case. | ||
before, after = text[:2], text[2:] | ||
text = f"{before}{after.upper()}" | ||
elif "e" in text: | ||
before, after = text.split("e") | ||
sign = "" | ||
if after.startswith("-"): | ||
after = after[1:] | ||
sign = "-" | ||
elif after.startswith("+"): | ||
after = after[1:] | ||
before = format_float_or_int_string(before) | ||
text = f"{before}e{sign}{after}" | ||
elif text.endswith(("j", "l")): | ||
number = text[:-1] | ||
suffix = text[-1] | ||
# Capitalize in "2L" because "l" looks too similar to "1". | ||
if suffix == "l": | ||
suffix = "L" | ||
text = f"{format_float_or_int_string(number)}{suffix}" | ||
elif is_hex(text): | ||
text = format_hex(text) | ||
elif is_scientific_notation(text): | ||
text = format_scientific_notation(text) | ||
elif is_long_or_complex_number(text): | ||
text = format_long_or_complex_number(text) | ||
else: | ||
text = format_float_or_int_string(text) | ||
leaf.value = text | ||
|
||
|
||
def is_oct_or_binary(text: str) -> bool: | ||
""" | ||
Checks if the supplied string is a number with octal or binary notation | ||
""" | ||
return text.startswith(("0o", "0b")) | ||
|
||
|
||
def is_hex(text: str) -> bool: | ||
"""Checks if the supplied string is a number with hexadecimal notation""" | ||
return text.startswith("0x") | ||
|
||
|
||
def format_hex(text: str) -> str: | ||
""" | ||
Formats a hexidecimal strign like "0x12b3" | ||
C0DK marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Uses lowercase because of similarity between "B" and "8", which | ||
can cause security issues. | ||
see: https://github.com/psf/black/issues/1692 | ||
""" | ||
|
||
before, after = text[:2], text[2:] | ||
return f"{before}{after.lower()}" | ||
|
||
|
||
def is_scientific_notation(text: str) -> bool: | ||
"""Checks if the supplied string is a number with scientific notation""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code matches 0xdeadbeef; it's an incorrect abstraction that depends on is_hex and other test having been applied earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid. Should i move all the checkers into the original function, while still moving the formatting into sub functions? Would that be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved these now :) |
||
return "e" in text | ||
|
||
|
||
def format_scientific_notation(text: str) -> str: | ||
"""Formats a numeric string utilizing scentific notation""" | ||
before, after = text.split("e") | ||
sign = "" | ||
if after.startswith("-"): | ||
after = after[1:] | ||
sign = "-" | ||
elif after.startswith("+"): | ||
after = after[1:] | ||
before = format_float_or_int_string(before) | ||
return f"{before}e{sign}{after}" | ||
|
||
|
||
def is_long_or_complex_number(text: str) -> bool: | ||
"""Checks if the supplied string is a long or complex number string""" | ||
return text.endswith(("j", "l")) | ||
|
||
|
||
def format_long_or_complex_number(text: str) -> str: | ||
"""Formats a long or complex string like `10L` or `10j`""" | ||
number = text[:-1] | ||
suffix = text[-1] | ||
# Capitalize in "2L" because "l" looks too similar to "1". | ||
if suffix == "l": | ||
suffix = "L" | ||
return f"{format_float_or_int_string(number)}{suffix}" | ||
|
||
|
||
def format_float_or_int_string(text: str) -> str: | ||
"""Formats a float string like "1.0".""" | ||
if "." not in text: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
pytest >= 6.0.1 | ||
pytest-mock >= 3.2.0 | ||
pytest-cases >= 2.1.2 | ||
coverage >= 5.2.1 | ||
coverage >= 5.2.1 | ||
|
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 don't think we need this refactoring; to me it just creates an excessive number of small functions.
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 is fair - I relied on the Uncle Bob principle of "If you need a comment, then you should probably just have a more descriptive function name", but i might be misquoting or misunderstanding that. It is definitely a subjective matter. I liked the, in my opinion, higher level of readability, but the, already long, file gets longer which is maybe not cool. We can also drop the whole refactoring commit (or only take the formatting parts).
It's subjective :) I'll go with what you guys prefer.
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.
It doesn't hurt to add a comment to some of these if statements. E.g.:
elif "e" in text: # scientific notation
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.
The function, in my opinion, does exactly the same without duplication and without revealing implementation details. Is a general consesus that the functions decrease quality? As mentioned, i explicitly added it in a seperate commit to make it easy to drop :)