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

precommit (black) rebased #218

Merged
merged 7 commits into from
Sep 25, 2020
Merged

precommit (black) rebased #218

merged 7 commits into from
Sep 25, 2020

Conversation

manics
Copy link
Member

@manics manics commented Sep 16, 2020

This is #207 with the following changes:

  • rebased
  • black applied
  • tests run natively instead of in Docker

This was referenced Sep 16, 2020
@will-moore
Copy link
Member

Great 👍 .
Can we get a badge in README like in napari?

[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/python/black)

will-moore and others added 6 commits September 22, 2020 15:03
Files have not been reformatted, need to run

    pre-commit run -a

to reformat all files with black and commit the result. flake8 should pass after this
pre-commit run --all-files
@manics
Copy link
Member Author

manics commented Sep 22, 2020

Second last commit changes the line length to 79 as suggested by @sbesson. Last commit are the manual flake8 changes required, this is because black can't split strings.

@sbesson
Copy link
Member

sbesson commented Sep 22, 2020

For some background reading on the line length discussion, the 79 characters used by flake8 by default is the maximal value that was originally suggested in PEP-008 while the more lenient 88 is defined by black and discussed in https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length.

As we are on the process of applying black across our Python repositories, this is probably one of the few implementation decisions we will have to decide as a team. As an element of comparison, a maximal line length of 88 results in +2.5K lines while a maximal length of 79 results in +4K lines.

@will-moore
Copy link
Member

I think line length of 88 is fine. It's a pain when you have things split onto more lines than necessary.

I noticed that before 757a7b4 that e.g. this original code:

    query = "select minIndex(ws), maxIndex(ws) from Well well " \
            "join well.wellSamples ws"

was changed to this (two adjacent strings) 👎

query = (
        "select minIndex(ws), maxIndex(ws) from Well well " "join well.wellSamples ws"
    )

With the shorter line length it's

query = (
    "select minIndex(ws), maxIndex(ws) from Well well "
    "join well.wellSamples ws"
)

which is better.
But in most cases the changes in 757a7b4 are annoying.

e.g.

-                    if sizex is None or sizey is None or (sizex * sizey) > limit:
+                    if (
+                        sizex is None
+                        or sizey is None
+                        or (sizex * sizey) > limit
+                    ):
                        can = False

@joshmoore
Copy link
Member

joshmoore commented Sep 23, 2020

Mark noticed several of the "..." "..." concatenations in my ome-zarr-py PRs. It's probably worth a regex check for these. (i.e. it can happen regardless of the line length)

@will-moore
Copy link
Member

Great, so I vote for longer line length and then we can follow-up with fixing "..." "...".

I see that when I enable "Format on Save" with black in VSCode, it also applies black to JavaScript files.
So that should probably come in a follow-up PR.

I'm going to want to open new PRs soon (see #223) and to un-exclude other PRs, so hopefully we can get this merged soon?

@manics
Copy link
Member Author

manics commented Sep 23, 2020

On balance I prefer the longer line length too, I tried the 79 limit on another repo and it seems to require more manual fixes than the default 88

@manics
Copy link
Member Author

manics commented Sep 23, 2020

@will-moore black is Python only, maybe vscode is using prettier? I think it's a good idea anyway

@manics
Copy link
Member Author

manics commented Sep 24, 2020

Consensus from today's discussion to be brought to the wider community:

  • Other autoformatters exist but black is a PSF project
  • Agreed 88 (black default) is acceptable
  • Configuration for Black and Flake8 in .pre-commit-config.yaml for now. This may differ for other tools, or if using separate configuration files enables text editors to auto-format code
  • Roll this policy out across all OME Python repos
  • Use pre-commit for other languages and repos too including Java
  • Need to communicate to other developers and the community when we're ready

@manics
Copy link
Member Author

manics commented Sep 24, 2020

I've forced pushed the 2 line-length-79 commits away, and added .git-blame-ignore-revs https://github.com/psf/black/blob/20.8b1/README.md#migrating-your-code-style-without-ruining-git-blame

@joshmoore
Copy link
Member

added .git-blame-ignore-revs

see also: https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256

@joshmoore
Copy link
Member

Merging since this is now blocking people. My understanding is that there's a general consensus that an auto-formatter is a good way forward and will ultimately lead to less churn rather than more. The line-length seems to be the least clear option, but worst case scenario it can be reduced here in a follow up PR which should only add changed-lines as @sbesson pointed out in #218 (comment) (2.5K --> 4K)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants