-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add formatter progress tracking to CI #5919
Conversation
195624c
to
9f5b6b8
Compare
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
PR Check ResultsBenchmarkLinux
Windows
|
2379138
to
804fc80
Compare
I think the main question remaining for me is: Should we pin the repos to a specific commit? pro: the score may otherwise change due to downstream changes unrelated to out ours Effectively, i want lockfiles and dependabot for this, but that doesn't exist, so we'll have to go with either option |
the result is now posted as github step summary (https://github.com/astral-sh/ruff/actions/runs/5636816309?pr=5919#summary-15269270283): i wish i could show that without switching to the CI tab and scrolling down, but it's still good to have this in CI either way |
I'm leaning toward pinning:
Nice: A future improvement could be integrating it into the same job that also posts the benchmark and ecosystem results |
.github/workflows/ci.yaml
Outdated
@@ -323,14 +323,19 @@ jobs: | |||
name: "Check formatter stability" | |||
runs-on: ubuntu-latest | |||
needs: determine_changes | |||
if: needs.determine_changes.outputs.formatter == 'true' | |||
# if: needs.determine_changes.outputs.formatter == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merge: Revert
.github/workflows/ci.yaml
Outdated
@@ -323,14 +323,19 @@ jobs: | |||
name: "Check formatter stability" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Rename, considering that it now checks more than just the progress.
scripts/formatter_progress.sh
Outdated
# build 5800521541e5e749d4429617420d1ef8cdb40b46 | ||
# django 0016a4299569a8f09ff24053ff2b8224f7fa4113 | ||
# transformers 5bb4430edc7df9f9950d412d98bbe505cc4d328b | ||
# typeshed 57c435cd7e964290005d0df0d9b5daf5bd2cbcb1 | ||
# warehouse e72cca94e7ac0dbe095db5c2942ad9f2f51b30cc | ||
# zulip 1cd587d24be1d668fcf6d136172bfec69e35cb75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
scripts/formatter_progress.sh
Outdated
cargo run --bin ruff_dev -- format-dev --stability-check --error-file "$target/progress_projects_errors.txt" \ | ||
--multi-project "$dir" >"$target/progress_projects_report.txt" | ||
grep "similarity index" "$target/progress_projects_report.txt" | sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: An alternative to grepping could have been to add a --markdown
flag to the CLI that enables a markdown rendered summary
a73c03e
to
f046dc5
Compare
Summary Add a formatter progress testing script to CI. This script will 1) print the black compability on each run 2) catch regressions wrt to formatter stability, emitting invalid syntax and other kinds of bugs (e.g. #5917) before they land on main 3) have an additional layer of real world tests when implementing new nodes or other new formatter code.
This is currently a bash script, i'm not sure if we want to keep it that way, or switch to e.g. the regular ecosystem scripts. The output separation of
format_dev
could also use some polishing. We should also consider pinning commits so we don't get spurious regression when they change their code.Test Plan The script extends CI.