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

configure pre-commit #7

Merged
merged 4 commits into from
Nov 21, 2024
Merged

configure pre-commit #7

merged 4 commits into from
Nov 21, 2024

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Nov 20, 2024

This adds a couple of pre-commit hooks:

  • trailing-whitespace, end-of-file-fixer, check-docstring-first: basic hooks
  • ruff: linting
  • prettier: format config files (mostly yaml)
  • taplo: format toml
  • validate-pyproject: validate pyproject.toml's project table

@keewis
Copy link
Collaborator Author

keewis commented Nov 20, 2024

no black / ruff-format, yet, since you asked me not to in #5 (comment). However, while it sure took me a while to get used to, I've come to love black: even if I would occasionally format things differently, it allows me to not worry about formatting and still get consistent formatting (which I find very valuable when reading other people's code).

Is your main objection the formatting of math (i.e. SPEC12), or are there other things as well? Because at least here I don't think we'd have complex math anywhere.

@mdhaber
Copy link
Owner

mdhaber commented Nov 21, 2024

No, it's mostly everything else : ). (SPEC 12 was mostly interesting as a theoretical exercise - if we could balance pretty formatting with relatively simple rules, what would they be? I don't currently follow it religiously. If it happens to be possible to get others onboard, it is something I'd be happy to follow, but we'll see.)

Another objection is that if I do start to like it, I won't be able to use it with SciPy, and that is where I will spend most of my time.

@mdhaber
Copy link
Owner

mdhaber commented Nov 21, 2024

Thanks for this!

@mdhaber mdhaber merged commit 6b65ba5 into mdhaber:main Nov 21, 2024
@mdhaber
Copy link
Owner

mdhaber commented Nov 21, 2024

If you could set up tests to run, too, that would be great!

Do you think we need to test with multiple libraries, or maybe just array_api_strict? I'm leaning toward just strict (unless it turns out to be too hard to get anything done). I'll try to convert the existing tests to use strict shortly.

@keewis keewis deleted the pre-commit branch November 21, 2024 09:28
@keewis
Copy link
Collaborator Author

keewis commented Nov 21, 2024

Another objection is that if I do start to like it, I won't be able to use it with SciPy, and that is where I will spend most of my time.

That's the situation I'm in, just in reverse 😅 I'll open a PR to add black (it's pretty easy to do), and if you really don't like it we can still close it without merging.

If you could set up tests to run, too, that would be great!

I was planning to! I just thought that pre-commit would be easier / faster to set up, in the little time I had left before going to sleep.

Do you think we need to test with multiple libraries, or maybe just array_api_strict?

array_api_strict should work, but when using it to test xarray I've noticed that the strictness comes with its own issues. Not sure how much that affects us here, but I think it could be good to also test with numpy. Also, for Array API compliance checks we might want to use the array-api-tests suite instead of rolling our own?

@keewis keewis mentioned this pull request Nov 21, 2024
@mdhaber
Copy link
Owner

mdhaber commented Nov 21, 2024

Also, for Array API compliance checks we might want to use the array-api-tests suite instead of rolling our own?

We also need to check that the masks are doing the right thing. We can at least have those in addition to what we write, or depending on how they are organized, it might be easier to extend them with masks than to start from scratch. What did you have in mind?

@keewis
Copy link
Collaborator Author

keewis commented Nov 21, 2024

nothing yet, I had not played around with array-api-tests, yet, I just know that it exists.

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.

2 participants