Skip to content

Conversation

@tobiasdiez
Copy link
Contributor

Add configuration for the ruff formatter to enforce single quotes for strings, otherwise it constantly reformats the strings to double-quotes.

Copy link
Member

@dnicolodi dnicolodi left a comment

Choose a reason for hiding this comment

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

I'm not a fan of formatters, but if others think that this is useful, I'm not opposed to it.

@rgommers
Copy link
Contributor

I'm fine with adding this setting too. Not a user myself, but if it helps others then a few lines of tool config doesn't hurt.

@eli-schwartz
Copy link
Member

Not directly involved in mesonpy development but FWIW...

I'm not a fan of autoformatters either. Actually, when someone does use an autoformatter and that autoformatter is named either "black" or "ruff", I will review their formatting like I review any other code and I will ask them to undo all changes made by those two tools since it looks really ugly.

While Meson does use single quotes, and telling an autoformatter to use single quotes produces output more aligned with Meson's coding style than use of the autoformatter without that setting, it's not enough to make it helpful when developing on Meson.

So, it does depend on whether the meson-python project devs view ruff as "eh, personally I wouldn't bother" or "please revert the style choices the autoformatter just made". In the former case, it may make sense to merge this PR. In the latter case, I'd suggest adding a ruff lint rule under the Q (flake8-quotes) section to check in CI that the correct project-specific quoting style is used, and auto fix them with ruff check --fix.

@eli-schwartz
Copy link
Member

eli-schwartz commented May 27, 2025

It may independently be worth configuring the linter to check the quoting style. Interestingly, ruff doesn't have a consistent configuration schema, so you will have to configure single quotes separately for the linter and the autoformatter or they will produce different results (format will produce code that check errors out on and autofixes to something that format then tries to revert again).

This isn't a bug, it's officially documented behavior that you have to set it twice. :/

@rgommers
Copy link
Contributor

So, it does depend on whether the meson-python project devs view ruff as "eh, personally I wouldn't bother" or "please revert the style choices the autoformatter just made"

What I thought this would be doing is align style choices for code already written by the author of a PR with what pre-commit enforces in CI. I don't touch pre-commit locally, so ruff formatting may cause less conflicts with style choices made in CI.

Either way, yeah the PR diffs should be minimal, that's always important - a formatting should never increase those.

@eli-schwartz
Copy link
Member

eli-schwartz commented May 27, 2025

What I thought this would be doing is align style choices for code already written by the author of a PR with what pre-commit enforces in CI. I don't touch pre-commit locally, so ruff formatting may cause less conflicts with style choices made in CI.

Either way, yeah the PR diffs should be minimal, that's always important - a formatting should never increase those.

Autoformatters traditionally discard all formatting in the file, converting the file to an abstract syntax tree and then re-rendering the entire file from scratch using its own internally preferred style (which I personally believe I have never actually seen anyone organically write -- it will never match any code other than something already processed by the autoformatter).

If any lines in the file still look the same after the autoformatter runs, it's because those lines happened to be what the autoformatter does anyway. Typically this is because the autoformatter is mandatorily enforced on every commit.

It's of course possible to hack around this with janky ecosystem wrappers that use git diff to detect the line number range that the PR modifies, seek out context markers for runs of code that are associated with the modified code (e.g. long tables of dictionary key/value pairs?) and then apply the autoformatter to a buffer. (This is implemented by the formatter as a --line-ranges=1-15 option, and is not idempotent!!! you will get different results depending on the order you format specific sections in, and thus will also produce different results compared to formatting the entire file at once.)

@tobiasdiez
Copy link
Contributor Author

It may independently be worth configuring the linter to check the quoting style.

That's already check if I'm not mistaken.

Autoformatters traditionally discard all formatting in the file

My strategy is usually to apply the formatter automatically on save but then only stage the lines relevant for the given PR.
I'm also culturally influenced by the js world, where autoformatters are way more common and less controversial.

@eli-schwartz
Copy link
Member

eli-schwartz commented May 29, 2025

It may independently be worth configuring the linter to check the quoting style.

That's already check if I'm not mistaken.

I don't see it in lint.select, are you sure?

I'm also culturally influenced by the js world, where autoformatters are way more common and less controversial.

Sure, but the js world has autoformatters written by people with more taste and less toxic ableism. :)

Black is simply bad at autoformatting, to the people that don't like it. For example, my personal belief is that black is a mixture of:

  • changes that autopep8 would already make
  • changes where black asked "what's the worst possible way to format", and then implemented that

with no actual exceptions. I wouldn't mind using an autoformatter if the results did what I wanted, rather than made my eyes bleed and destroyed my interest in contributing at all to projects that use that formatting. The point of tools is to automate stuff you want to do, not automate stuff you don't want to do. The JavaScript world understands that -- the python world doesn't (and probably never will).

@dnicolodi
Copy link
Member

It may independently be worth configuring the linter to check the quoting style.

That's already check if I'm not mistaken.

I don't see it in lint.select, are you sure?

It's enforced by pre-commit-hook, I think here

- id: double-quote-string-fixer
Having it enforced by ruff too would not be a bad idea, just to make it easier to check for the ones of us that do not buy into the pre-commit-hook thing.

@dnicolodi
Copy link
Member

Please squash the commits.

@eli-schwartz
Copy link
Member

It's enforced by pre-commit-hook, I think here

- id: double-quote-string-fixer

Having it enforced by ruff too would not be a bad idea, just to make it easier to check for the ones of us that do not buy into the pre-commit-hook thing.

Ohhhh I see.

I think it would be worth moving that, yes, given that the pre-commit-hook thing already seems to be running ruff, and that would also speed it up because it doesn't have to run as many distinct shell commands (one per id of that hooks collection thing).

@dnicolodi dnicolodi changed the title MAINT: Add config for ruff formatter MAINT: add config for ruff formatter May 29, 2025
@dnicolodi
Copy link
Member

See #757

@dnicolodi dnicolodi merged commit 6414f05 into mesonbuild:main May 30, 2025
41 checks passed
@tobiasdiez tobiasdiez deleted the ruff-format branch May 30, 2025 05:12
@rgommers rgommers added this to the v0.19.0 milestone May 31, 2025
@rgommers rgommers added the maintenance Regular code improvements that are not new features nor end-user-visible bugs label May 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Regular code improvements that are not new features nor end-user-visible bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants