Skip to content

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jul 11, 2025

When I switched this repo over to ruff, I botched an important detail. It is split into two parts, a linter (think Flake8) and a formatter (think Black). At the time, I neglected to add the formatter to automation.

I tried to do this with minimal disruptions, but there just isn't a straightforward way to do that in such a large codebase. Most (estimate 70%+) of the changes are re-wrapping some things that Black was eager to unwrap in the past. This is evidenced by how the changes should be identical but the codebase is reduced by ~3000 LOC. (I'm fine with this, line lengths of ~80 are too narrow to me on modern displays.) This can be tinkered with by changing the line length config, but I left that in place.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Comment on lines -8 to +9
- id: ruff
- id: ruff-format
- id: ruff-check
Copy link
Member Author

Choose a reason for hiding this comment

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

This diff masks two separate changes

  • adding ruff-format, which did most of this PR
  • renaming ruff to ruff-check, which is a change that happened sometime in the past few months. (See their boilerplate config.) If you saw something like ruff (legacy alias) locally, this is why

Copy link

codecov bot commented Jul 11, 2025

Codecov Report

❌ Patch coverage is 73.03922% with 110 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.64%. Comparing base (d70974d) to head (83aa50a).
⚠️ Report is 1 commits behind head on main.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mattwthompson mattwthompson marked this pull request as ready for review July 11, 2025 16:54
@mattwthompson mattwthompson requested a review from j-wags as a code owner July 11, 2025 16:54
@j-wags j-wags self-assigned this Jul 14, 2025
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Preliminarily approved, but let's hold off on merging until the NAGLCharges stuff is done so we don't get the merge-conflict-pocalypse

@j-wags
Copy link
Member

j-wags commented Aug 14, 2025

I'm resolving conflicts and will merge this before the 0.17.0 reelase

@j-wags
Copy link
Member

j-wags commented Aug 14, 2025

Oh, some of these conflicts are substantial-ish. Would it be easy to reopen this PR from a fresh reformatting of main or is that a lot of manual work?

@mattwthompson
Copy link
Member Author

I'll have a look

@mattwthompson
Copy link
Member Author

I have the tooling happy (so far) via accepting all upstream changes, re-running the formatter, and removing duplicate code.

@j-wags j-wags merged commit 9f29507 into main Aug 14, 2025
17 checks passed
@j-wags j-wags deleted the actually-format-ruff branch August 14, 2025 19:59
@j-wags
Copy link
Member

j-wags commented Aug 14, 2025

Thanks @mattwthompson!

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