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

[FEA] Investigate using black #23

Closed
jakirkham opened this issue May 5, 2021 · 4 comments · Fixed by #616
Closed

[FEA] Investigate using black #23

jakirkham opened this issue May 5, 2021 · 4 comments · Fixed by #616
Labels
feature request New feature or request

Comments

@jakirkham
Copy link
Member

Previously we tried to use black for formatting Python, but ran into some issues. It would be good to revisit this at some point

cc @grlee77 @quasiben

@grlee77
Copy link
Contributor

grlee77 commented May 5, 2021

I think the two issues are:
1.) unless fmt:off / fmt:on blocks are used, it can make some small test matrices formatted in a less readable way.
2.) since upstream scikit-image is not formatted with black, it makes direct comparison with upstream code in diff tools a bit more difficult

@jakirkham
Copy link
Member Author

Yeah the matrix point makes sense. Disabling formatting on them seems fine

On 2 it might be worth asking scikit-image if they are willing to use black. We started using this a while back in Dask + Distributed as well as RAPIDS, which has helped keep things readable and consistent

@grlee77
Copy link
Contributor

grlee77 commented May 5, 2021

On 2 it might be worth asking scikit-image if they are willing to use black.

It did come up in the past in scikit-image/scikit-image#4111 and wasn't received too favorably at the time. Maybe people are more familiar with it by now, though?

I am not too strongly against using it here even if upstream does not. We can always apply black to the scikit-image source prior to doing a diff if we need to compare files directly across the two. For specific files like colorconv.py where we added custom CUDA kernel code, the diff will already be pretty large in either case.

@jakirkham
Copy link
Member Author

Thanks for the context. Yeah I think black has also softened on some things, which helps, and more projects have started to use it, which also helps issue discovery & subsequent fixes

That said, if it is going to make it hard to coordinates changes between here and scikit-image. It's probably not worth the effort until scikit-image adopts it

@rapids-bot rapids-bot bot closed this as completed in #616 Oct 27, 2023
rapids-bot bot pushed a commit that referenced this issue Oct 27, 2023
…ect.toml (#616)

The first commit here is from #615

This MR makes multiple linting-related changes to cuCIM
- moves existing `isort` config from `setup.cfg` to a new `pyproject.toml` file
- update `isort` conventions to match other RAPIDS projects
- add `black` and apply it (use `fmt::off`/`fmt::on` as needed to preserve formatting of look-up tables and test cases)
- replace use of `flake8` with `ruff`
- add config for `codespell` and apply recommended fixes across both C++ and Python code
- update CONTRIBUTING.md instructions to reflect these changes
- Pre-commit hooks for `black`, `ruff` and `codespell` were added. The pre-commit hook for `flake8` was removed.

The only non-linting change is
- move pytest configuration from `setup.cfg` to `pyproject.toml`

I plan to leave any build-system related changes to `pyproject.toml` to a follow-up MR. This MR focuses only on linting/testing.

It will probably be easiest to review the individual commits. Some, like applying `black` are quite large.

Fixes #23

Authors:
  - Gregory Lee (https://github.com/grlee77)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ray Douglass (https://github.com/raydouglass)
  - Gigon Bae (https://github.com/gigony)
  - https://github.com/jakirkham

URL: #616
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants