Use lowercase hex numbers fixes #1692 #1775

C0DK · 2020-10-21T16:07:38Z

First PR onto black. Please tell me if i did something wrong according to your ordinary rules.

The issue, #1692, argues whether we/you should "simply" do this, as it might impact source code of a large set of users, however you can either merge it if you like it or wait if not. :) Please write me if I can do anything in that regard.

I created two seperate commits:

Made hex lower case
Refactored numeric formatting section

This was done to seperate the two focus points, the first one solves the issue, whereas the second adds a bit of refactoring to that part of the code. Whether my version has a higher code quality or not, is definitely subjective, so please ignore it and cherry-pick if you dislike it :)

I am looking forward to contributing to black, long time user, and all that, so I hope you like it. I'll try to do something once in a while - Am doing this as part of hacktoberfest though, so if you can either label it hacktoberfest-accepted or merge it before the end of the month, that will be great ;) I feel super cheap writing that, but i am trying to pimp my bike with a yearly sticker from hacktoberfest, and this month is running a bit short. If any changes are needed, I'll gladly make those :)

Thank you!

ichard26

Didn't do a deep check on the code, but I did notice a few stylistic things.

ichard26 · 2020-10-21T20:02:46Z

src/black/__init__.py

+# All possible string prefix characters.
+STRING_PREFIX_CHARS: Final = "furbFURB"


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

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

Fixed (also all the rest)

ichard26 · 2020-10-21T20:03:04Z

src/black/__init__.py

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


ichard26 · 2020-10-21T20:03:24Z

src/black/__init__.py

+            # Line is empty, don't emit. Creating a new one unnecessary.
+            return


ichard26 · 2020-10-21T20:05:04Z

.gitignore

+.venv/
+
+


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

Will fix. Thank you.

ichard26 · 2020-10-21T20:49:29Z

.venv

@@ -0,0 +1 @@
+black


What is this file for?

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

JelleZijlstra · 2020-10-22T07:56:34Z

src/black/__init__.py

        # Leave octal and binary literals alone.
        pass
-    elif text.startswith("0x"):


I don't think we need this refactoring; to me it just creates an excessive number of small functions.

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.

It doesn't hurt to add a comment to some of these if statements. E.g.:

elif "e" in text: # scientific notation

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

hugovk · 2020-10-22T08:19:51Z

test_requirements.txt

+coverage >= 5.2.1
+aiohttp >= 3.3.2
+aiohttp-cors


Are these two additions needed?

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

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?

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

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?

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

https://github.com/hynek/structlog/blob/69d9e9328d86cdc5e395eadd119c9775ee7343f7/setup.py#L41-L49

But that's for another PR.

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

hugovk · 2020-10-23T07:25:38Z

For the Primer failures:

The CI is running Black on a few other projects to see if the formatting changes. Sometimes we don't want that to happen. In this case, some changes are expected.

I checked a log, 5 projects now fail and they all look like hex number changes, as expected.

To fix: flip expect_formatting_changes": false, to true in src/black_primer/primer.json for attrs, hypothesis, Pillow, pyramid and virtualenv.

C0DK · 2020-10-23T09:51:05Z

@hugovk Thank you :) Should be fixed now.

Regarding my refactoring, show i revert those? What is the general consesus? I can always revert it and then put it in another PR

ichard26 · 2020-10-23T21:02:24Z

I completely forgot but please also add an entry in the change log, thank you!

kamahen · 2020-10-23T21:25:42Z

src/black/__init__.py

+
+
+def is_scientific_notation(text: str) -> bool:
+    """Checks if the supplied string is a number with scientific notation"""


This code matches 0xdeadbeef; it's an incorrect abstraction that depends on is_hex and other test having been applied earlier.

Valid. Should i move all the checkers into the original function, while still moving the formatting into sub functions? Would that be better.

I moved these now :)

cooperlees

LGTM - But want other core devs completely happy too.

C0DK · 2020-10-28T08:37:11Z

LGTM - But want other core devs completely happy too.

I'll update the changelog in less than 12 hours.

I hope i can get a hacktoberfest-approved label on this by the end of the month :p if you are desperately against hacktoberfest, then i understand :) I enjoy it as a reminder to give back to the community, meanwhile sticker-hunting is a desperate hobby of mine.

zsol · 2020-10-28T22:08:24Z

LGTM. To make me completely happy, I'd also revert the refactoring that creates three tiny functions that @JelleZijlstra pointed out.

C0DK · 2020-10-29T09:04:45Z

LGTM. To make me completely happy, I'd also revert the refactoring that creates three tiny functions that @JelleZijlstra pointed out.

Whichh are these? I did revert the smallest of functions, but kept the formatting functions, as the numeric formatting already was in it's own function. This keeps it on the same level of abstraction, IMO.

src/black/__init__.py

zsol · 2020-10-29T13:51:56Z

I did revert the smallest of functions, but kept the formatting functions, as the numeric formatting already was in it's own function. This keeps it on the same level of abstraction, IMO.

You're right, the formatting functions are fine. I was referring to the functions that only called startswith("0x") and such. All good now.

Co-authored-by: Zsolt Dollenstein <zsol.zsol@gmail.com>

C0DK · 2020-10-30T13:28:33Z

I've mentioned it before, but didn't get any response: Will you add a hacktoberfest label? :P It's fine if you guys don't want to encourage it - I totally respect that. I'm just a bit pressed if i want to partake this year otherwise haha.

cooperlees · 2020-10-30T15:30:05Z

I've mentioned it before, but didn't get any response: Will you add a hacktoberfest label? :P It's fine if you guys don't want to encourage it - I totally respect that. I'm just a bit pressed if i want to partake this year otherwise haha.

Is this label all you need?

C0DK · 2020-10-30T20:48:39Z

Is this label all you need?

hacktoberfest-accepted - to be really annoying ;)

This reverts commit 7d032fa.

* Made hex lower case * Refactored numeric formatting section * Redid some refactoring and removed bloat * Removed additions from test_requirements.txt * Primer now expects expected changes * Undid some refactoring * added to changelog * Update src/black/__init__.py Co-authored-by: Zsolt Dollenstein <zsol.zsol@gmail.com> Co-authored-by: Zsolt Dollenstein <zsol.zsol@gmail.com> Co-authored-by: Cooper Lees <me@cooperlees.com>

Casper Weiss Bang added 2 commits October 21, 2020 18:01

Made hex lower case

da6743f

Refactored numeric formatting section

a607ada

ichard26 reviewed Oct 21, 2020

View reviewed changes

JelleZijlstra reviewed Oct 22, 2020

View reviewed changes

hugovk reviewed Oct 22, 2020

View reviewed changes

Casper Weiss Bang added 2 commits October 22, 2020 13:27

Redid some refactoring and removed bloat

0758897

Removed additions from test_requirements.txt

d7339e8

Primer now expects expected changes

ffff687

kamahen reviewed Oct 23, 2020

View reviewed changes

Undid some refactoring

4aa80bc

cooperlees approved these changes Oct 27, 2020

View reviewed changes

added to changelog

831343b

zsol reviewed Oct 29, 2020

View reviewed changes

src/black/__init__.py Outdated Show resolved Hide resolved

Update src/black/__init__.py

cf07525

Co-authored-by: Zsolt Dollenstein <zsol.zsol@gmail.com>

This comment has been minimized.

Sign in to view

Merge branch 'master' into issue-1692

84554f6

cooperlees added the Hacktoberfest label Oct 30, 2020

zsol added hacktoberfest-accepted and removed Hacktoberfest labels Oct 31, 2020

Merge branch 'master' into issue-1692

dd80843

cooperlees merged commit 7d032fa into psf:master Nov 13, 2020

C0DK deleted the issue-1692 branch November 14, 2020 10:56

Jackenmen mentioned this pull request Mar 26, 2021

Numeric literal formatting is inconsistent with Black code style document #2066

Closed

ambv added a commit that referenced this pull request Apr 25, 2021

Revert "Use lowercase hex numbers fixes #1692 (#1775)"

773e4a2

This reverts commit 7d032fa.

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

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

		# Line is empty, don't emit. Creating a new one unnecessary.
		return



		def is_scientific_notation(text: str) -> bool:
		"""Checks if the supplied string is a number with scientific notation"""

		.venv/

Use lowercase hex numbers fixes #1692 #1775

Use lowercase hex numbers fixes #1692 #1775

Conversation

C0DK commented Oct 21, 2020

ichard26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk commented Oct 23, 2020

C0DK commented Oct 23, 2020

ichard26 commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cooperlees left a comment

Choose a reason for hiding this comment

C0DK commented Oct 28, 2020

zsol commented Oct 28, 2020

C0DK commented Oct 29, 2020 • edited

zsol commented Oct 29, 2020

C0DK commented Oct 30, 2020

This comment has been minimized.

cooperlees commented Oct 30, 2020

C0DK commented Oct 30, 2020

C0DK commented Oct 29, 2020 •

edited