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

Add pre-commit config with a few hooks and mdformat #38

Merged
merged 14 commits into from
Jul 26, 2024

Conversation

lysnikolaou
Copy link
Collaborator

No description provided.

@lysnikolaou
Copy link
Collaborator Author

This is ready to be reviewed. What do you think @rgommers @ngoldbaum @Fidget-Spinner @andfoy? Are you happy with these hooks/checks?

@rgommers
Copy link
Member

I'm honestly not sure. The main changes to C code are:

  • PYMODINIT_FUNC which is made less idiomatic (all examples in the CPython code have it as two lines as well, e.g.,: https://docs.python.org/3/extending/extending.html)
  • adding a lot of spacing under PyModuleDef_Slot module_slots, which looks worse
  • making the int do_modification a one-liner, which looks a little worse
  • having to declare it's not C but all C++, I guess to make the checker run.

The yaml changes look fine. Line breaks are sometimes worse, sometimes better. It's only a few changes, so I don't have strong feelings either way. I'll leave it to others, who will probably be writing more of the C code examples than me, to decide whether to merge.

Copy link
Collaborator

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the bot is auto-fixing this stuff I don't mind much, but I don't think the changes are actually improving much, so maybe not worth the hassle?

docs/ci.md Show resolved Hide resolved
docs/ci.md Show resolved Hide resolved
docs/porting.md Outdated Show resolved Hide resolved
docs/tracking.md Show resolved Hide resolved
@lysnikolaou
Copy link
Collaborator Author

Agreed with both your sentiments, I also have mixed feelings about this.

My proposal would be to get rid of clang-format, it's not a big improvement and if we want it to format the "right" way (like CPython/NumPy etc. do), we'll have to maintain a more complicated clang-format file which isn't really worth it.

I like having the rest in place, if only for not really caring about trailing whitespace, keeping formatting consistent in .md files etc.

@lysnikolaou
Copy link
Collaborator Author

I think this is ready for a final decision.

@ngoldbaum
Copy link
Collaborator

Another argument for doing this is that we can add codespell, which currently has a high signal-to-noise ratio on the docs:

$ codespell docs
docs/porting.md:179: occure ==> occur, occurred

(I'll fix the spelling issue in a separate PR)

@lysnikolaou
Copy link
Collaborator Author

Good point. Though codespell might not be able to always autofix (for example doing codespell -w docs does not auto-fix the "occure" spelling mistake).

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have two in favor and no one against, so let's just give this a go! Thanks @lysnikolaou and @ngoldbaum.

@rgommers rgommers merged commit ccb5198 into Quansight-Labs:main Jul 26, 2024
2 checks passed
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.

3 participants