Skip to content
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

Rewrite ecosystem checks and add ruff format reports #8223

Merged
merged 76 commits into from
Oct 27, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 25, 2023

Closes #7239

  • Refactors scripts/check_ecosystem.py into a new Python project at python/ruff-ecosystem
  • Fixes bug where ruff check report included "fixable" summary line
  • Adds truncation to ruff check reports
    • Otherwise we often won't see the ruff format reports
    • The truncation uses some very simple heuristics and could be improved in the future
  • Identifies diagnostic changes that occur just because a violation's fix available changes
    • We still show the diff for the line because it's could matter where this changes, but we could improve this
    • Similarly, we could improve detection of diagnostic changes where just the message changes
  • Adds support for JSON ecosystem check output
    • I added this primarily for development purposes
  • If there are no changes, only errors while processing projects, we display a different summary message
  • When caching repositories, we now checkout the requested ref
  • Adds ruff format reports, which format with the baseline then the use format --diff to generate a report
  • Runs all CI jobs when the CI workflow is changed

Known problems

  • Since we must format the project to get a baseline, the permalink line numbers do not exactly correspond to the correct range
    • This looks... hard. I tried using git diff and some wonky hunk matching to recover the original line numbers but it doesn't seem worth it. I think we should probably commit the formatted changes to a fork or something if we want great results here. Consequently, I've just used the start line instead of a range for now.
  • I don't love the comment structure — it'd be nice, perhaps, to have separate headings for the linter and formatter.
    • However, the pr-comment workflow is an absolute pain to change because it runs separately from this pull request so I if I want to make edits to it I can only test it via manual workflow dispatch.
  • Lines are not printed "as we go" which means they're all held in memory, presumably this would be a problem for large-scale ecosystem checks
  • We are encountering a hard limit with the maximum comment length supported by GitHub. We will need to move the bulk of the report elsewhere.

Future work

  • Update ruff-ecosystem to support non-default projects and check_ecosystem_all.py behavior
  • Remove existing ecosystem check scripts
  • Add preview mode toggle (Run ecosystem checks with and without preview enabled #8076)
  • Add a toggle for truncation
  • Add hints for quick reproduction of runs locally
  • Consider parsing JSON output of Ruff instead of using regex to parse the text output
  • Links to project repositories should use the commit hash we checked against
  • When caching repositories, we should pull the latest changes for the ref
  • Sort check diffs by path and rule code only (changes in messages should not change order)
  • Update check diffs to distinguish between new violations and changes in messages
  • Add "fix" diffs
  • Remove existing formatter similarity reports
  • On release pull request, compare to the previous tag instead

@zanieb zanieb force-pushed the zanie/ecosystem-format branch from 0202efd to ea0602a Compare October 25, 2023 18:21
@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

✅ ecosystem check detected no format changes.

@zanieb zanieb force-pushed the zanie/ecosystem-format branch 3 times, most recently from fd7fc90 to 5f1ef59 Compare October 25, 2023 19:11
@zanieb zanieb added the internal An internal refactor or improvement label Oct 25, 2023
@zanieb zanieb force-pushed the zanie/ecosystem-format branch 2 times, most recently from dddeffe to 2f1b233 Compare October 25, 2023 20:05
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I heaven't read all the code but I find it much easier to quickly identify the relevant code parts that are responsible or X compared to before.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

very clean code!


def __add__(self, other: Self) -> Self:
if not isinstance(other, type(self)):
return NotImplemented
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
return NotImplemented
return TypeError

That's TRY004 :P

Copy link
Member Author

Choose a reason for hiding this comment

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

I think not here https://docs.python.org/3.11/library/constants.html#NotImplemented

Although raise TypeError would be fine, this is "more correct"

@MichaReiser
Copy link
Member

very clean code!

That means a lot if comes from @konstin !

@zanieb zanieb force-pushed the zanie/ecosystem-format branch from f5a8feb to 63f1122 Compare October 27, 2023 17:08
@zanieb zanieb marked this pull request as ready for review October 27, 2023 20:33
@zanieb zanieb merged commit fc94857 into main Oct 27, 2023
@zanieb zanieb deleted the zanie/ecosystem-format branch October 27, 2023 22:28
zanieb added a commit that referenced this pull request Oct 28, 2023
Fixes bug where `total_affected_rules` is empty, a division by zero
error can occur if there are only errors and no rule changes. Calculates
the maximum display per rule with the calculated project maximum as the
upper bound instead of 50, this should show more rule variety when
project maximums are lower.

This commit was meant to be in #8223 but I missed it.
zanieb added a commit that referenced this pull request Oct 29, 2023
Changes the title and adds some notes re the old formatter ecosystem
checks in light of #8223

Does not remove it as I'm not sure where else we test for instabilities.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooling: Ruff-shades
3 participants