-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Move to pyproject.toml from setup.{py/cfg} and add a minimal ruff linter/formatter #526
base: main
Are you sure you want to change the base?
Conversation
Another thing is that I have removed the |
Please no, we are talking about turning off more formatting stuff than we already have and this goes the opposite direction. But I let @msdemlei to chime in, too. |
No worries, we don't have to add more linters :) Is consolidating everything in pyproject.toml okay? I think it really helps to have everything in one place as a single source of truth. |
On Tue, Feb 20, 2024 at 12:47:24PM -0800, Brigitta Sipőcz wrote:
> Add a ruff linter and formatter to pre-commit to bring the linting in this repo up to date with astropy/astropy
Please no, we are talking about turning off more formatting stuff
than we already have and this goes the opposite direction. But I
let @msdemlei to chime in, too.
Well... I freely admit that my favourite passage in PEP 8 is
However, know when to be inconsistent – sometimes style guide
recommendations just aren’t applicable. When in doubt, use your
best judgment. Look at other examples and decide what looks best.
And indeed, in my code that others don't generally contribute to, I'm
quite a distance remote from PEP 8 (starting with my uncurable
affection for tabs).
On the other hand, I totally see that we want to minimise the number
of diff lines only dealing with whitespace and formatting, and so I
certainly won't veto committing to as many conventions as are
necessary to achieve that goal. Going beyond that, I'm more
skeptical. And I am particularly skeptical about having a machine
re-format source code. But maybe that's just the result of having
been bitten by inferior tools of yesteryear.
So: If all it takes for ruff-ing things up is a few dozen lines of
diff, I won't say no. If the result is thousands of lines of diff,
I'd pull a Bartleby ("I would prefer not to").
|
I had a look at it, and the ruff reformatting touches ~6k lines like this. With a bit of rules selections (not forcing the strings to be double quoted was the most important gain), I could get down to ~3k lines diff. Ex: __all__ = [
- "parse_tables", "parse_capabilities", "parse_availability",
- "TablesFile", "CapabilitiesFile", "AvailabilityFile"]
+ "parse_tables",
+ "parse_capabilities",
+ "parse_availability",
+ "TablesFile",
+ "CapabilitiesFile",
+ "AvailabilityFile",
+] See the open issue astral-sh/ruff#11886 in Ruff. The only way to disable it in the formatting for now is to add a My take on this would be to drop the Ruff formatting from this PR until this issue is solved. We could still add the existing codestyle check to a pre-commit hook if everyone agrees. |
On Thu, Mar 20, 2025 at 06:40:58AM -0700, Manon Marchand wrote:
My take on this would be to drop the Ruff formatting from this PR
until this issue is solved.
+1 (and I don't think I find these minor formatting standardisations
useful enough to bother at all)
|
I started working on #155 and I thought it would be helpful to clean up this repo a bit before.
I have moved around a bunch of stuff in this PR, let me know if it makes easier to review if I break this up in smaller pull requests :)
I hope I didn't break anything here 😅
python -m build
was able to build a wheel successfully and I was able to runpytest
with no failures.I have not run the ruff linter yet! Once we have a positive review I can run the pre-commit bits to format the code (didn't want to pollute the diff too much).