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

Changes to black formatting #149

Open
petercorke opened this issue Jan 8, 2025 · 9 comments
Open

Changes to black formatting #149

petercorke opened this issue Jan 8, 2025 · 9 comments

Comments

@petercorke
Copy link
Collaborator

I've noticed that black on my system is folding lines differently to what is in the repo, despite that also being formatted by black.

For example in base/vectors.py line 622 was

@overload
def angdiff(a: ArrayLike, b: ArrayLike) -> NDArray:
    ...

but black now formats it like

@overload
def angdiff(a: ArrayLike, b: ArrayLike) -> NDArray: ...

There's a discussion about this here. I'm using 24.8.0 from Aug 2024. TBH I like the new format better, but this is going to throw a lot cosmetic noise into the commits which is annoying -- the whole point of black was supposed to be stability and inflexibility.

Would appreciate discussion on this. I think there are only 2 options:

  • pin the version of black that all contributors use, impossible to enforce
  • accept that there will be a lot of unnecessary changes for a while until things settle down
@petercorke petercorke mentioned this issue Jan 12, 2025
@myeatman-bdai
Copy link
Collaborator

myeatman-bdai commented Jan 14, 2025

Ah, just getting eyes on this.

We added a .pre-commit-config.yaml file that calls out the specific version of black and the other pre commit hooks (this was copied and paired down from what we use internally in our big repos). My apologies if this generated some friction. I think this gets to the enforcing a version of black for contributors. Running pre-commit install from the root of the repo should get people setup.

@petercorke
Copy link
Collaborator Author

pre-commit install does cause the formatting to be fixed up on commit, but I'm using a Visual Studio extension which blacks code on every save. So code formatting keeps reverting back to the non standard one.

So for any people (4.2M users) using this extension it's important to change the default setting:

Screenshot 2025-01-15 at 10 43 06 am

@petercorke
Copy link
Collaborator Author

I will remove/modify PR #152 and resubmit it without all the gratuitous format changes.

@petercorke
Copy link
Collaborator Author

I'm trying to now commit the properly blacked files, and the pre-commit hooks kicked in (good thing) but now I can't commit (bad thing):

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

.
.
.

Found 819 errors (4 fixed, 815 remaining).
No fixes available (4 hidden fixes can be enabled with the `--unsafe-fixes` option).

black....................................................................Failed
- hook id: black
- duration: 1.64s
- files were modified by this hook

reformatted spatialmath/base/transforms3d.py

All done! ✨ 🍰 ✨
1 file reformatted, 5 files left unchanged.

fix end of files.........................................................Passed
debug statements (python)................................................Passed
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing spatialmath/base/argcheck.py
Fixing spatialmath/base/transforms2d.py
Fixing spatialmath/base/transforms3d.py

[INFO] Restored changes from /Users/pic/.cache/pre-commit/patch1736902349-99096.

ruff is mostly fixated on import *, not best practice, but pervasive in this package :(. It also doesn't seem to understand the @overload syntax for different type signatures.

I don't see any ruff configuration options in pyproject.toml, but I'm guessing you are not seeing this kind of behaviour.

I tried adding to pyproject.toml

[tool.ruff]
unsafe-fixes = true

but there was no change in ruff behaviour.

The black and trailing white space commands did their job, so their fails got turned into passes on subsequent commit attempts.

I've not used pre-commits or ruff before.

@myeatman-bdai
Copy link
Collaborator

I'm getting the same issue as you are when I try to commit the changes in PR 152 to a branch, a spew about star imports and the like. I generally haven't had this issue in the past, I'm not sure what has changed. I'm investigating now.

@myeatman-bdai
Copy link
Collaborator

I think what's happened is that we added this pre-commit-config.yaml and then we (people at the institute) just haven't been actually installing it when we've been making changes to the repo. I'm going to make a quick PR to remove the pre-commit check, and then later we can have a big reformatting PR or the like to resolve this.

@petercorke
Copy link
Collaborator Author

For those who actually did the pre-commit install you still need to uninstall

pre-commit uninstall -t pre-commit

before you nuke the .pre-commit-config.yaml file so it knows what to uninstall.

Just removing the file, without uninstalling doesn't fix the problem.

@petercorke
Copy link
Collaborator Author

Could one of the BDAI crew apply all the outstanding PRs and then run black 23.10 on the whole repo. It'll make life a whole lot easier for devs.

@myeatman-bdai
Copy link
Collaborator

Yes, I will give it a shot.

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

No branches or pull requests

2 participants