Use Black for Formatting #94

meawoppl · 2022-05-05T00:44:52Z

A quick demo of how to make black auto-format everything. This also shifts format checking to gh infrastructure.

All I did to generate a bulk of the changes in this PR was:
black picamera2 which in turn output the reformatted code:

This has the side benefit of killing a lot of diff noise between PR's

Signed-off-by: Matthew Goodman matt@exclosure.io

meawoppl · 2022-05-05T00:47:19Z

Note that style check now takes about... 4 seconds:

davidplowman · 2022-05-05T07:51:43Z

Hi, thanks for posting this it looks interesting. Can you say how I could enforce this kind of style during development, for example by using the git pre-commit hook? Also, have you changed the style checking process on the test machine to complain if people don't stick to this style or not, I didn't quite understand that.

meawoppl · 2022-05-05T15:54:27Z

There are a number of options for this:

Before commiting run black picamera2 from the repository root
Install black and add black picamera2 to the .git/hooks/pre-commit
Install and use the pre-commit framework (which is what I do in my projects), which will manage installation of many potential pre-commit options (including black). It's nice because it comes with a raft of options that you can scale in as the code becomes more mature. Starting with formatting is easy, then adding lint, doc requirements, type-checking etc all just becomes a couple lines of config

Here is what that looks like in a project I am working on:

meawoppl · 2022-05-05T15:59:06Z

Also, this PR is likely to be conflicted by basically any other change, so let me know when/if you want it and I can freshen it :)

meawoppl · 2022-05-05T19:43:34Z

And to answer your earlier question. This line will cause the CI to reject PR's that are not consistant with the formatting requirements.

meawoppl · 2022-05-05T21:15:15Z

As a totally incidental side benefit, this takes the style check responsibility off the pi you are using to run tests :)

naushir · 2022-05-06T08:34:50Z

This change seems reasonable, but there are a couple of things that I can see we need to address:

The github action command probably wants to be black --check --diff picamera2 to show what formatting changes would be needed.
There does not seem to be a way to run black on a diff/patch as far as I can tell. This is not ideal, since if we chose to ignore a particular style dictated by the tool for a single PR, the tool will end up complaining about it for all subsequent PRs, which defeats the purpose of the pre-commit check.

meawoppl · 2022-05-11T01:19:20Z

Hmmm. The flags are easy to switch out. Let me start there.

Following up on the second bullet point. Looking through the checker it seems that it completely checks all files/lines, then filters to lines contained in the diff only. I am more than a little confused as to how that can deal with multi-line expressions in a sane way, but that isn't the question here. black is named for the Henry Ford quote, its kinda hostile by design.

It might just not meet the needs of this group if y'all want to have exceptionally formatted sections.

FWIW, the not having redo PR's for code style issues is about 50% of the appeal, another 40% is not having to educate folks that contribute about the particular flavor of code style. 🤷‍♂️

Signed-off-by: Matthew Goodman <matt@exclosure.io>

naushir · 2022-05-12T12:01:40Z

Apparently running on a diff has been a feature request for some time now (psf/black#830, psf/black#142, psf/black#245, psf/black#370, psf/black#511). Looks unlikely that this will be put into mainline.

There are a few wrapper-of-sorts that does seem to do this - https://github.com/akaihola/darker and https://github.com/wbolster/black-macchiato. Perhaps it's worth looking to see if those could be used instead? In particular, darker looks very promising and might be pretty much a drop-in replacement for the existing script.

naushir

Here are some comments on the formatting produced by Black. Of course, these preferences are entirely personal to my tastes.

naushir · 2022-05-12T12:15:25Z

picamera2/converters.py

+)
+YUV2RGB_REC709 = np.array(
+    [[1.164, 1.164, 1.164], [0.0, -0.213, 2.112], [1.793, -0.533, 0.0]]
+)



I think I prefer the original formatting. Or possibly all 3 matrices to look something like:

YUV2RGB_REC709 = np.array( [ [1.164, 1.164, 1.164], [0.0, -0.213, 2.112], [1.793, -0.533, 0.0] ] )

naushir · 2022-05-12T12:17:01Z

picamera2/encoders/jpeg_encoder.py

-        return simplejpeg.encode_jpeg(array, quality=self.q, colorspace=self.colour_space)
+        return simplejpeg.encode_jpeg(
+            array, quality=self.q, colorspace=self.colour_space
+        )


Not sure how I feel about the formatting here and in all the other places where the function arguments are put on the next line. The original reads easier to me.

naushir · 2022-05-12T12:18:29Z

picamera2/picamera2.py

-        self.lock = threading.Lock()  # protects the functions and completed_requests fields
+        self.lock = (
+            threading.Lock()
+        )  # protects the functions and completed_requests fields
        self.have_event_loop = False


The original looks better and more concise.

I think it was just trying to maintain the comment on the same line here...

naushir · 2022-05-12T12:19:54Z

picamera2/picamera2.py

-        if not isinstance(camera_config["colour_space"], libcamera._libcamera.ColorSpace):
+        if not isinstance(
+            camera_config["colour_space"], libcamera._libcamera.ColorSpace
+        ):
            raise RuntimeError("Colour space has incorrect type")


Prefer the original.

naushir · 2022-05-12T12:20:22Z

picamera2/picamera2.py

+            "YUYV",
+            "UYVY",
+            "VYUY",
+        )



I think the original single line version looks better.

naushir · 2022-05-12T12:21:03Z

picamera2/previews/gl_helpers.py

+    pointer,
+    CFUNCTYPE,
+    c_bool,
+)



I think the original single line version looks better.

naushir · 2022-05-12T12:21:33Z

picamera2/previews/q_gl_picamera2.py

+            EGL_ALPHA_SIZE,
+            0,
+            EGL_RENDERABLE_TYPE,
+            EGL_OPENGL_ES2_BIT,
            EGL_NONE,
        ]


The original looks better as you can associate the key-value pairs on one line. Ditto for the instances below.

Oh yeah, that is kinda and uglee tranform. Maybe these want to be a dict then tranformed at the call site?

meawoppl · 2022-05-12T21:39:16Z

Seems like this isn't a real improvement here.

meawoppl changed the title ~~Formatting/checking based on black~~ Use Black for Formatting May 5, 2022

meawoppl added 2 commits May 10, 2022 18:20

Black all files.

d0ac478

Signed-off-by: Matthew Goodman <matt@exclosure.io>

Add the --diff flag as suggested

ea7563d

naushir reviewed May 12, 2022

View reviewed changes

meawoppl closed this May 12, 2022

meawoppl mentioned this pull request Nov 3, 2022

encoders import tidy. #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Black for Formatting #94

Use Black for Formatting #94

meawoppl commented May 5, 2022 •

edited

meawoppl commented May 5, 2022

davidplowman commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

naushir commented May 6, 2022

meawoppl commented May 11, 2022

naushir commented May 12, 2022 •

edited

naushir left a comment

naushir May 12, 2022 •

edited

naushir May 12, 2022

naushir May 12, 2022 •

edited

meawoppl May 12, 2022

naushir May 12, 2022

naushir May 12, 2022

meawoppl May 12, 2022

naushir May 12, 2022

naushir May 12, 2022

meawoppl May 12, 2022

meawoppl commented May 12, 2022 •

edited

Use Black for Formatting #94

Use Black for Formatting #94

Conversation

meawoppl commented May 5, 2022 • edited

meawoppl commented May 5, 2022

davidplowman commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

meawoppl commented May 5, 2022

naushir commented May 6, 2022

meawoppl commented May 11, 2022

naushir commented May 12, 2022 • edited

naushir left a comment

Choose a reason for hiding this comment

naushir May 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naushir May 12, 2022 • edited

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

meawoppl commented May 12, 2022 • edited

meawoppl commented May 5, 2022 •

edited

naushir commented May 12, 2022 •

edited

naushir May 12, 2022 •

edited

naushir May 12, 2022 •

edited

meawoppl commented May 12, 2022 •

edited