Skip to content
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

Merged
merged 10 commits into from Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
@@ -1,3 +1,6 @@
.venv/


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if the two extra empty lines were removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

.coverage
_build
.DS_Store
Expand All @@ -15,4 +18,4 @@ src/_black_version.py
.dmypy.json
*.swp
.hypothesis/
venv/
venv/
1 change: 1 addition & 0 deletions .venv
@@ -0,0 +1 @@
black
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this file for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was an error. sorry. should have been caught by the gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

96 changes: 71 additions & 25 deletions src/black/__init__.py
Expand Up @@ -69,7 +69,8 @@
DEFAULT_INCLUDES = r"\.pyi?$"
CACHE_DIR = Path(user_cache_dir("black", version=__version__))

STRING_PREFIX_CHARS: Final = "furbFURB" # All possible string prefix characters.
# All possible string prefix characters.
STRING_PREFIX_CHARS: Final = "furbFURB"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: IMO this is unnecessary and just ruins blame information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was a result of autoformatter. I see the point. will revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (also all the rest)



# types
Expand Down Expand Up @@ -1901,7 +1902,8 @@ def line(self, indent: int = 0) -> Iterator[Line]:
"""
if not self.current_line:
self.current_line.depth += indent
return # Line is empty, don't emit. Creating a new one unnecessary.
# Line is empty, don't emit. Creating a new one unnecessary.
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto


complete_line = self.current_line
self.current_line = Line(depth=complete_line.depth + indent)
Expand Down Expand Up @@ -5174,35 +5176,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"):
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link

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

Copy link
Contributor Author

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

# 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"""
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
Expand Down Expand Up @@ -5296,7 +5341,8 @@ def convert_one_fmt_off_pair(node: Node) -> bool:
if not ignored_nodes:
continue

first = ignored_nodes[0] # Can be a container node with the `leaf`.
# Can be a container node with the `leaf`.
first = ignored_nodes[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

parent = first.parent
prefix = first.prefix
first.prefix = prefix[comment.consumed :]
Expand Down
2 changes: 1 addition & 1 deletion src/blib2to3/pytree.py
Expand Up @@ -34,7 +34,7 @@
import sys
from io import StringIO

HUGE: int = 0x7FFFFFFF # maximum repeat count, default max
HUGE: int = 0x7fffffff # maximum repeat count, default max

_type_reprs: Dict[int, Union[Text, int]] = {}

Expand Down
4 changes: 3 additions & 1 deletion test_requirements.txt
@@ -1,4 +1,6 @@
pytest >= 6.0.1
pytest-mock >= 3.2.0
pytest-cases >= 2.1.2
coverage >= 5.2.1
coverage >= 5.2.1
aiohttp >= 3.3.2
aiohttp-cors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these two additions needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - i was not able to run all the tests witout them :) Some tests rely on AIO stuff, so when i installed them i added them as well :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, CONTRIBUTING.md suggests using pipenv, which includes them.

I'm not so familiar with pipenv, but I see Travis CI runs pip install -e '.[d]', where the d is a so-called "extra" listed in setup.py:

    extras_require={
        "d": ["aiohttp>=3.3.2", "aiohttp-cors"],
        "colorama": ["colorama>=0.4.3"],
    },

So that will install them for you. And GitHub Actions uses tox which also installs with the extra.

Want to try running pip install -e '.[d]' instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK this requirements file is for CI not developers of Black. Doing a Pipenv developement install should install Black with its HTTP web server dependencies.

black/Pipfile

Line 21 in 4070527

black = {editable = true, extras = ["d"], path = "."}

You get pinned dependencies too so doing a Pipenv development install is recommended

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oic. I use some...thing.. not actually sure. I simply realized it wasn't exhaustive, not that it was defined elsewhere. Should i remove these two lines?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another popular thing to do is to move tests dependencies from test_requirements.txt into a tests extra, for example:

But that's for another PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think remove those two lines to avoid duplication and keep things lean 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :)

4 changes: 2 additions & 2 deletions tests/data/numeric_literals.py
Expand Up @@ -12,7 +12,7 @@
x = 123456789E123456789
x = 123456789J
x = 123456789.123456789J
x = 0XB1ACC
x = 0Xb1aCc
x = 0B1011
x = 0O777
x = 0.000000006
Expand All @@ -36,7 +36,7 @@
x = 123456789e123456789
x = 123456789j
x = 123456789.123456789j
x = 0xB1ACC
x = 0xb1acc
x = 0b1011
x = 0o777
x = 0.000000006
Expand Down
4 changes: 2 additions & 2 deletions tests/data/numeric_literals_py2.py
Expand Up @@ -3,7 +3,7 @@
x = 123456789L
x = 123456789l
x = 123456789
x = 0xb1acc
x = 0xB1aCc

# output

Expand All @@ -13,4 +13,4 @@
x = 123456789L
x = 123456789L
x = 123456789
x = 0xB1ACC
x = 0xb1acc
6 changes: 3 additions & 3 deletions tests/data/numeric_literals_skip_underscores.py
Expand Up @@ -3,7 +3,7 @@
x = 123456789
x = 1_2_3_4_5_6_7
x = 1E+1
x = 0xb1acc
x = 0xb1AcC
x = 0.00_00_006
x = 12_34_567J
x = .1_2
Expand All @@ -16,8 +16,8 @@
x = 123456789
x = 1_2_3_4_5_6_7
x = 1e1
x = 0xB1ACC
x = 0xb1acc
x = 0.00_00_006
x = 12_34_567j
x = 0.1_2
x = 1_2.0
x = 1_2.0