Add Python formatting with Ruff

[hoodmane]

No description provided.

[npaun]

I don't see a ruff config file in the PR. Shouldn't we add your config?

[danlapid]

Make sure to run the formatter and commit the changed files

[hoodmane]

Well I want to see the CI fail first.

[hoodmane]

Shouldn't we add your config?

I think the configuration I suggested before is all configuring the lints for ruff check, for ruff format I think we can just use the defaults and it will work fine. I looked through the list of ruff format configuration options and none of them look that interesting to me.

[hoodmane]

I think I'll add ruff linting in a separate PR.

[npaun]

Don't forget to add the relevant commit to .git-blame-ignore-revs

[anonrig]

Can we have a .python-version file in the root of this project, so we don't have to use python-version: CONSTANT in everywhere?

[hoodmane]

To be honest, I only see 3.12 repeated in two places, so I'm not sure this would be helpful.

[fhanau]

FWIW the Ubuntu 24.04 dev image defaults to python 3.12, so this is currently a no-op (https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#language-and-runtime). There's a case for keeping it though to use a fixed python version even if we update the build image, or if we want to update to python 3.13 early (comes out in 6 weeks).

[anonrig]

This is also an indicator for IDEs to automatically use a specific version of Python whenever the interpreter is not configured. I wish this change was included in this pull-request.

[anonrig]

Can we use ruff-action? https://github.com/ChartBoost/ruff-action

[hoodmane]

Well with the current approach it works locally for people to do python ./tools/cross/format.py if they have ruff installed, whereas if we used the action there wouldn't be a good local approach. I think @danlapid said he'd prefer to modify format.py over using ci-specific automation.

[anonrig]

ruff-action is used to install ruff. it is not used to execute ruff. right now, we will always use the latest ruff version.

[danlapid]

Do we have any kind of "verbose" flag for ruff so we see what files it formats or checks?

[anonrig]

Actually, I was just about to open a pull-request to remove it, since it reduces formatting performance. Any particular reason to enable it?

[npaun]

I find it convenient for local use to know which files were checked, and if they needed their formatting corrected or not.

[danlapid]

Agree with @npaun also, formatting shouldn't take too long for regular runs where a user just has a few files staged for commits.
The CI run might take a while but it's gonna be faster than build&test anyway.

[danlapid]

@anonrig What kind of time difference did you see between with verbose vs without?

[anonrig]

I've opened a pull-request about 2 changes that I think will fulfill everybody's needs. #2576

@anonrig What kind of time difference did you see between with verbose vs without?

I'm not sure. I have to benchmark it using a tool like hyperfine to came up with a scientific result.

[hoodmane]

There is a --verbose flag but it is so noisy that you would only use it if you are debugging ruff. I think the standard verbosity is reasonable. Seems like we want to use --diff and not --check though:

      --check
          Avoid writing any formatted files back; instead, exit with a non-zero status code if any files would have been modified, and zero otherwise

      --diff
          Avoid writing any formatted files back; instead, exit with a non-zero status code and the difference between the current file and how the formatted file would look like

[danlapid]

Yea sounds right.

[hoodmane]

I find it convenient for local use to know which files were checked, and if they needed their formatting corrected or not.

Unfortunately I don't think that ruff has a mode that does this without printing way too much output.

[danlapid]

I don't have any opinion on the following matter but I think it should be brought up and discussed:
In c++ code we use indentation size of 2 spaces.
Should we be consistent with python intention size as well?

[npaun]

PEP 8 recommends 4 spaces for Python, so that's my personal preference

[hoodmane]

Yeah the vast majority of Python code in the wild uses 4 spaces.

[npaun]

Commenting just in case: Make sure to merge this PR via "Merge" and not "Squash". Otherwise, .git-blame-ignore-revs will not work properly.

[hoodmane]

I wish it were possible to disable squash merge ahead of time on a particular PR, I have a tendency to accidentally squash merge patch release PRs and things like that.

[npaun]

Yeah, same here.