Skip to content

Conversation

ReeceHumphreys
Copy link
Contributor

Description

  1. Runs the existing pre-commit tool over entire repo
  2. Runs Ruff format over entire repo and updates pre-commit tool to format python with Ruff
  3. Runs Clang-format over entire repo and updates pre-commit tool to format cpp with Clang-Format

The entire repo will now adhere to these formatters making our code much more consistent. The pre commit tool runs in CI/CD so no new improperly formatted code should get into the repo moving forward too.

Verification

It's basically impossible to manually check all the code given how many files get changed. CI/CD should catch anything and formatting changes should not be impacting any logic.

@Mark2000 any rules you changed for bsk-rl with Ruff? I ideally like to keep formatters as close to default as possible.

Documentation

N/A

Future work

N/A

Closes #1109

@ReeceHumphreys ReeceHumphreys self-assigned this Sep 22, 2025
@ReeceHumphreys ReeceHumphreys requested a review from a team as a code owner September 22, 2025 17:24
@ReeceHumphreys ReeceHumphreys added the refactor Clean up with no new functionality label Sep 22, 2025
@juan-g-bonilla
Copy link
Contributor

I'd like to review line by line, please allow for 1-2 years for review thanks. Just joking.

I see the formatter was applied to all files, including things like .mtl, .obj, .svg, .sty... That's probably fine (?) but a bit weird since those files usually have their own whitespace/formatting requirement or they come from external data sources and we don't necessarily want to change their SHA/checksum.

@schaubh
Copy link
Contributor

schaubh commented Sep 22, 2025

I don't think we want to do this to the full repo. What is the reason versus doing this module by module as we edit it? This will create a TON of potential conflicts with people who pull or fork our repo?

@ReeceHumphreys
Copy link
Contributor Author

This was just from running the existing pre-commit tool on the repo. I agree we should discuss is we want to change any of the rules!

This is not a hard PR to do as it was really just running the tool against the repo and updating pre-commit to include clang-format and Ruff. Moreso wanted a kick off a discussion about it.

For people who want to rebase our repo it actually is a bit easier than one may think. Because everything is handled via the pre-commit tool all anyone would need to do is first run pre-commit run --all-files (how I did this PR) and then merge in any of our changes. That way any formatting changes are already addressed their end.

@ReeceHumphreys
Copy link
Contributor Author

I'd like to review line by line, please allow for 1-2 years for review thanks. Just joking.

I see the formatter was applied to all files, including things like .mtl, .obj, .svg, .sty... That's probably fine (?) but a bit weird since those files usually have their own whitespace/formatting requirement or they come from external data sources and we don't necessarily want to change their SHA/checksum.

Yeh I agree, I think this is because pre-commit run --all-files will just run the existing white space and line ending checks on every single file type. Probably is a way to reduce that.

@ReeceHumphreys
Copy link
Contributor Author

Will move this to a draft will in person discussions are pending

@ReeceHumphreys ReeceHumphreys marked this pull request as draft September 22, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Clean up with no new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint and Format entire repository
3 participants