style: add ruff and black to pre-commit #1459

d-v-b · 2023-07-12T13:57:00Z

This PR is a prelude to reformatting the codebase with ruff and black linters. .pre-commit-config.yaml and pyproject.toml have been modified to add + configure ruff and black (and remove flake8).

A subsequent PR will contain the styling changes.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

d-v-b · 2023-07-12T14:00:57Z

😵‍💫 it looks like the code gets automatically styled by github actions after modifying the pre-commit config, so it's not possible to separate the config changes from the styling itself in separate PRs.

codecov · 2023-07-12T14:10:48Z

Codecov Report

Merging #1459 (fd144c4) into main (8fc3b4b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1459   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines        14723     14723           
=========================================
  Hits         14723     14723

Impacted Files	Coverage Δ
zarr/context.py	`100.00% <ø> (ø)`
zarr/tests/test_meta_array.py	`100.00% <ø> (ø)`
zarr/tests/test_storage.py	`100.00% <ø> (ø)`
zarr/__init__.py	`100.00% <100.00%> (ø)`
zarr/_storage/absstore.py	`100.00% <100.00%> (ø)`
zarr/_storage/store.py	`100.00% <100.00%> (ø)`
zarr/_storage/v3.py	`100.00% <100.00%> (ø)`
zarr/_storage/v3_storage_transformers.py	`100.00% <100.00%> (ø)`
zarr/attrs.py	`100.00% <100.00%> (ø)`
zarr/convenience.py	`100.00% <100.00%> (ø)`
... and 24 more

d-v-b · 2023-07-12T14:11:56Z

I'm not sure what to think of the pre-commit-ci bot. I feel like linting should be part of the checking process, not automatically applied via automated commits. At least for this PR, automatically running pre-commit is very wrong, because I deliberately did not commit the files that the bot modified.

edit: it also seems like the bot is not running pre-commit in the same way as in my local environment. When I execute pre-commit run --all-files on the commit produced by the bot, pre-commit fails.

local pre commit run

(zarr) ➜  zarr-python git:(ruff_black_prelude) pre-commit run --all-files
black....................................................................Passed
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

zarr/_storage/absstore.py:52:101: E501 Line too long (141 > 100 characters)
zarr/_storage/v3.py:18:5: F401 `zarr._storage.store._listdir_from_keys` imported but unused
zarr/_storage/v3.py:19:5: F401 `zarr._storage.store._rename_from_keys` imported but unused
zarr/_storage/v3.py:21:5: F401 `zarr._storage.store._rmdir_from_keys` imported but unused
zarr/_storage/v3.py:23:5: F401 `zarr._storage.store._path_to_prefix` imported but unused
zarr/_storage/v3.py:26:5: F401 `zarr._storage.store.array_meta_key` imported but unused
zarr/_storage/v3.py:27:5: F401 `zarr._storage.store.attrs_key` imported but unused
zarr/_storage/v3.py:29:5: F401 `zarr._storage.store.group_meta_key` imported but unused
zarr/hierarchy.py:331:24: E713 Test for membership should be `not in`
zarr/storage.py:73:5: F401 `zarr._storage.store._rename_metadata_v3` imported but unused
zarr/util.py:227:20: E741 Ambiguous variable name: `l`
Found 11 error(s).
9 potentially fixable with the --fix option.

codespell................................................................Failed
- hook id: codespell
- exit code: 65

WARNING: Cannot decode file using encoding "utf-8": fixture/8/0/32
WARNING: Trying next encoding "iso-8859-1"
WARNING: Cannot decode file using encoding "utf-8": fixture/8/0/11
WARNING: Trying next encoding "iso-8859-1"
WARNING: Cannot decode file using encoding "utf-8": fixture/8/0/23
WARNING: Trying next encoding "iso-8859-1"
WARNING: Cannot decode file using encoding "utf-8": fixture/8/0/28
WARNING: Trying next encoding "iso-8859-1"
WARNING: Cannot decode file using encoding "utf-8": fixture/0/0/4
WARNING: Trying next encoding "iso-8859-1"
WARNING: Cannot decode file using encoding "utf-8": fixture/8/0/19
WARNING: Trying next encoding "iso-8859-1"
zarr/tests/test_storage_v3.py:167: zar ==> czar
WARNING: Cannot decode file using encoding "utf-8": fixture/0/0/9
WARNING: Trying next encoding "iso-8859-1"

check yaml...............................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

zarr/_storage/store.py:308: error: List item 0 has incompatible type "None"; expected "Union[bytes, memoryview, bytearray]"  [list-item]
zarr/hierarchy.py:150: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
zarr/hierarchy.py:158: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Found 1 error in 1 file (checked 40 source files)

d-v-b · 2023-07-12T14:26:59Z

@rabernat any thoughts on how to proceed here? Because of the pre-commit-ci bot, there's no way to cleanly separate the styling config changes from the styling itself in git history if we squash the commits in this PR. We could just not squash the commits, and I then exempt the CI bot's commit from git blame?

Saransh-cpp · 2023-07-12T15:19:36Z

#1460 (comment)

and, you can use ruff --fix to eliminate the need for using black, plus ruff --fix should be a lot faster than black 🙂

Saransh-cpp

Also, you don't actually need a separate PR to preserve git blame. You could do something like -

first commit - adding the new pre-commit, new config, etc.
second commit - the changes introduced by the new pre-commit (let the bot commit in the PR or fix them manually by pre-commit run --all-files)
third commit - update .git-blame-ignore-revs with the commit hash of the second commit (and probably a comment explaining why the commit is ignored)

d-v-b · 2023-07-12T15:26:22Z

thanks for the pointers @Saransh-cpp! Do you have any idea why the bot is running pre-commit successfully, while my local runs produce a lot of errors?

Saransh-cpp · 2023-07-12T15:30:55Z

The bot does show a few errors too, here - https://results.pre-commit.ci/run/github/48049137/1689170255.FwDDg2OQSPy25V6evetUAQ. The bot just fixes everything that it can fix and then leaves the things that have to be manually fixed. If there are things that it cannot fix, it errors out, and the logs are available in the GH Actions section of PRs.

Regarding #1460 (comment): Linting is also a part of the checks. If you scroll down in the GH Actions or the checks section of this PR, you'll find a pre-commit.ci check. Having the bot is a personal choice, some projects have it, and some don't, completely depends on the maintainers. I personally like it because it fixes PRs, runs the pre-commit check using pre-commit.ci (which is faster than running it on GH Actions), and periodically updates the versions of packages in pre-commit-config.yaml.

d-v-b · 2023-07-12T15:33:48Z

but this is confusing, (and maybe i just don't understand pre-commit well enough), because I thought the entire point of pre-commit hooks were that they block you from committing if they fail. So it's not obvious why the bot is committing against my branch when the pre-commit hooks are failing.

Saransh-cpp · 2023-07-12T15:39:01Z

pre-commit will block you from committing locally (if you have installed it using pre-commit install in the repository), but the bot fixes the commits if someone pushes without installing the pre-commit hooks locally. Just like sometimes people push without testing their changes locally and GH Actions runs the tests for them. But again, pre-commit and the bot are personal preferences, all your points are valid, I know a lot of projects that don't use the bot because of the reasons specified by you.

d-v-b · 2023-07-12T15:40:32Z

the bot and I are not off to a good start, but maybe i will change my mind about it. In the meantime, I will try to work around it. thanks for your help so far!

d-v-b · 2023-07-12T18:23:46Z

@Saransh-cpp I'm trying to avoid triggering this bot. Can you explain what I did wrong in my commit message?

commit 499f45ea8149eacffa717b63b7afc4384f30b18c (HEAD -> ruff_black_prelude, origin/ruff_black_prelude)
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Wed Jul 12 18:21:40 2023 +0000

    style: pre-commit fixes

commit 9c8137a2cbb7c820ea595a457c7fd3257cda894b
Author: Davis Vann Bennett <davis.v.bennett@gmail.com>
Date:   Wed Jul 12 09:54:21 2023 -0400

    style: add ruff and black to pre-commit
    
    [pre-commit.ci skip]

d-v-b · 2023-07-12T18:37:30Z

I'm locking this branch so that the bot cannot make any more changes.

d-v-b · 2023-07-13T02:34:20Z

@rabernat I think this is ready. sorry for all the noise!

it looks like the repo is configured to only allow PRs via the squash and merge strategy, which will obviate the addition to .git-blame-ignore-revs. Could we do a merge commit for just this PR? Alternatively I can fix the git blame stuff in a subsequent PR.

rabernat · 2023-07-13T03:02:14Z

Could we do a merge commit for just this PR?

I am absolutely fine with that. However, I'd like to check with @joshmoore before changing the repo settings.

rabernat

Overall this looks good to me. It's hard to actually "review" a PR that touches nearly every file in the code base. My impression is that the pre-commit config looks right and the CI is working. The formatting changes all seem reasonable.

it would be great if someone with deeper git knowledge (e.g. @jakirkham) to weigh in on what this will do to our commit log. I don't foresee any problems, but I could be missing something.

Thanks so much @d-v-b for taking on this important chore!

joshmoore · 2023-07-13T11:49:57Z

Here are my thoughts:

use of .git-blame-ignore-revs definitely helps but isn't a total panacea
as I mentioned to @d-v-b elsewhere (perhaps by voice but perhaps on one of his other PRs), my primary concern with this is the impact on outstanding PRs. That's the primary trade-off in my opinion that needs to be weighed.

rabernat · 2023-07-13T12:33:19Z

my primary concern with this is the impact on outstanding PRs

Are there any specific PRs you have in mind? My sense is that there are not a lot of active PRs right now, which might make it a good time to do this change. It will never be trivial, but we have to rip the bandaid somehow.

joshmoore · 2023-07-15T20:19:32Z

Are there any specific PRs you have in mind?

Not particularly, no. But I also haven't reviewed in a while. If you guys want to run with this, I'd ask you to take responsibility for one of:

closing stale PRs with an appropriate comment
communicating with the authors and seeing if they want to fix the conflicts
fixing the conflicts (e.g. by ruff'ing each of the PRs??)

cc: @jakirkham

rabernat · 2023-07-17T13:09:26Z

@d-v-b - I merged #1462 before this, which has now introduced conflicts. If you can fix those, we can merge this today.

…into ruff_black_prelude

d-v-b · 2023-07-17T13:32:58Z

@rabernat conflicts are resolved.

rabernat · 2023-07-18T16:06:02Z

For some reason codedov status has not been submitted. It would be great to have that go green before merging. Does anyone know what's going on? It looks like codecov has not received a payload for d27a6aa.

https://app.codecov.io/gh/zarr-developers/zarr-python/pull/1459

Edit: seeing this in the workflow logs

[2023-07-17T13:28:49.416Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.1&token=*******&branch=ruff_black_prelude&build=5576131092&build_url=https%3A%2F%2Fgithub.com%2Fzarr-developers%2Fzarr-python%2Factions%2Fruns%2F5576131092&commit=d27a6aa6e7b561af266c3b15d0478ac9caa6f2bc&job=Linux+Testing&pr=1459&service=github-actions&slug=zarr-developers%2Fzarr-python&name=&tag=&flags=&parent=
[2023-07-17T13:28:49.416Z] ['verbose'] Passed token was 0 characters long
[2023-07-17T13:28:49.417Z] ['verbose'] https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.1&branch=ruff_black_prelude&build=5576131092&build_url=https%3A%2F%2Fgithub.com%2Fzarr-developers%2Fzarr-python%2Factions%2Fruns%2F5576131092&commit=d27a6aa6e7b561af266c3b15d0478ac9caa6f2bc&job=Linux+Testing&pr=1459&service=github-actions&slug=zarr-developers%2Fzarr-python&name=&tag=&flags=&parent=
        Content-Type: 'text/plain'
        Content-Encoding: 'gzip'
        X-Reduced-Redundancy: 'false'
[2023-07-17T13:28:49.711Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
[2023-07-17T13:28:49.711Z] ['verbose'] The error stack is: Error: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

This reverts commit 9043fb7.

d-v-b · 2023-07-18T17:53:29Z

@joshmoore

Not particularly, no. But I also haven't reviewed in a while. If you guys want to run with this, I'd ask you to take responsibility for one of:
* closing stale PRs with an appropriate comment

* communicating with the authors and seeing if they want to fix the conflicts

* fixing the conflicts (e.g. by ruff'ing each of the PRs??)

Maybe I don't understand the issue here -- won't authors of active PRs be prompted to pull in the latest version of main, which will include the .pre-commit config, which they can then use on their branches?

joshmoore · 2023-07-18T18:35:30Z

I've spent a good deal of time (previously) going through PRs and trying to keep them up-to-date. I don't necessarily expect you guys to do that, but I do think communicating what's going on would be a kind thing to do for these other contributors. Not everyone will be using pre-commit locally and if the automated check is disabled there won't be any indication of this new requirement.

d-v-b · 2023-07-18T18:38:36Z

I'm happy to go through and update extant PRs as needed.

Without squashing to simplify the history management.

github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 12, 2023

d-v-b mentioned this pull request Jul 12, 2023

pre-commit-ci bot #1460

Open

Saransh-cpp reviewed Jul 12, 2023

View reviewed changes

d-v-b force-pushed the ruff_black_prelude branch 2 times, most recently from 95fa611 to 9c8137a Compare July 12, 2023 18:21

style: add ruff and black to pre-commit

2d9e687

d-v-b force-pushed the ruff_black_prelude branch from 499f45e to 2d9e687 Compare July 12, 2023 18:39

d-v-b added 3 commits July 12, 2023 22:15

style: tweak codespell config to avoid a false positive

94cdd1a

style: lint the codebase

4e348d6

chore: add linting commit to list of revs ignored for git blame

b79f1e2

docs: update release notes

b0c50e1

github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jul 13, 2023

rabernat approved these changes Jul 13, 2023

View reviewed changes

Merge branch 'main' of https://github.com/zarr-developers/zarr-python …

d27a6aa

…into ruff_black_prelude

jhamman approved these changes Jul 17, 2023

View reviewed changes

Merge branch 'main' into ruff_black_prelude

fd144c4

rabernat merged commit 9043fb7 into zarr-developers:main Jul 18, 2023
21 checks passed

rabernat added a commit that referenced this pull request Jul 18, 2023

Revert "style: add ruff and black to pre-commit (#1459)"

fb190b4

This reverts commit 9043fb7.

rabernat mentioned this pull request Jul 18, 2023

Revert "style: add ruff and black to pre-commit" #1469

Closed

joshmoore added a commit that referenced this pull request Jul 19, 2023

style: add ruff and black to pre-commit (#1459)

13cdbff

Without squashing to simplify the history management.

This was referenced Jul 19, 2023

Updated release notes for 2.16.0 #1471

Merged

Codecov uploads failing #1472

Closed

jakirkham mentioned this pull request Aug 22, 2023

Disable pre-commit.ci's PR autofixes #1506

Merged

6 tasks

DimitriPapadopoulos mentioned this pull request Sep 13, 2023

Change occurrences of % and format() to f-strings #1423

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: add ruff and black to pre-commit #1459

style: add ruff and black to pre-commit #1459

d-v-b commented Jul 12, 2023 •

edited

d-v-b commented Jul 12, 2023

codecov bot commented Jul 12, 2023 •

edited

d-v-b commented Jul 12, 2023 •

edited

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023 •

edited

Saransh-cpp left a comment

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023 •

edited

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 13, 2023 •

edited

rabernat commented Jul 13, 2023

rabernat left a comment

joshmoore commented Jul 13, 2023

rabernat commented Jul 13, 2023 •

edited

joshmoore commented Jul 15, 2023

rabernat commented Jul 17, 2023

d-v-b commented Jul 17, 2023

rabernat commented Jul 18, 2023 •

edited

d-v-b commented Jul 18, 2023

joshmoore commented Jul 18, 2023

d-v-b commented Jul 18, 2023

style: add ruff and black to pre-commit #1459

style: add ruff and black to pre-commit #1459

Conversation

d-v-b commented Jul 12, 2023 • edited

d-v-b commented Jul 12, 2023

codecov bot commented Jul 12, 2023 • edited

Codecov Report

d-v-b commented Jul 12, 2023 • edited

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023 • edited

Saransh-cpp left a comment

Choose a reason for hiding this comment

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023 • edited

d-v-b commented Jul 12, 2023

Saransh-cpp commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 12, 2023

d-v-b commented Jul 13, 2023 • edited

rabernat commented Jul 13, 2023

rabernat left a comment

Choose a reason for hiding this comment

joshmoore commented Jul 13, 2023

rabernat commented Jul 13, 2023 • edited

joshmoore commented Jul 15, 2023

rabernat commented Jul 17, 2023

d-v-b commented Jul 17, 2023

rabernat commented Jul 18, 2023 • edited

d-v-b commented Jul 18, 2023

joshmoore commented Jul 18, 2023

d-v-b commented Jul 18, 2023

d-v-b commented Jul 12, 2023 •

edited

codecov bot commented Jul 12, 2023 •

edited

d-v-b commented Jul 12, 2023 •

edited

Saransh-cpp commented Jul 12, 2023 •

edited

Saransh-cpp commented Jul 12, 2023 •

edited

d-v-b commented Jul 13, 2023 •

edited

rabernat commented Jul 13, 2023 •

edited

rabernat commented Jul 18, 2023 •

edited