Type stats follow renaming#5805
Conversation
3fd76e5 to
4d05ff2
Compare
4d05ff2 to
258e456
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 258e456f4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path: path.to_s, | ||
| line: ignore.line | ||
| } | ||
| ignored_source = source_lines[ignore.line - 1]&.strip |
There was a problem hiding this comment.
Distinguish block steep ignores by their ignored code
When the suppression is a block form (# steep:ignore:start), ignore.line points at the start comment itself, so ignored_source is also just that comment. Since the new comparison key intentionally ignores line numbers, two block suppressions with the same start comment in the same file become indistinguishable; removing one block and adding an unrelated block elsewhere in that file will cancel out and the typing-stats comment will hide the newly introduced suppression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I agree, now that we're not comparing with line numbers anymore but the content of the ignored lines, we should probably use everything that is between the steep:ignore:start and steep:ignore:end (which would also correctly report any change between start and end). But maybe this should be for another PR
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6331a4e | Docs | Datadog PR Page | Give us feedback! |
vpellan
left a comment
There was a problem hiding this comment.
Awesome work, thank you! I never really had time to go back to this tooling, but this should prevent a lot of noise due to renaming files or adding/removing lines. I still requested a change because as-is, this PR might not work as expected (see my comment in AST traversal)
| path: path.to_s, | ||
| line: ignore.line | ||
| } | ||
| ignored_source = source_lines[ignore.line - 1]&.strip |
There was a problem hiding this comment.
I agree, now that we're not comparing with line numbers anymore but the content of the ignored lines, we should probably use everything that is between the steep:ignore:start and steep:ignore:end (which would also correctly report any change between start and end). But maybe this should be for another PR
| ::RBS::AST::Declarations::Class, | ||
| ::RBS::AST::Declarations::Interface | ||
| ast_traversal(declaration.members, result) | ||
| ast_traversal(declaration.members, result, context + [declaration.name.to_s]) |
There was a problem hiding this comment.
When you rename a file, you'll usually also rename the corresponding Module, including in the .rbs file, which means the context will be different. I don't see any operation on context in typing_stats_compare.rb so I assume it will report them as two different declaration and not fix the issue you're trying to solve here. We could probably normalize the file name, and when comparing the context, check every module name against their corresponding renamed one (although there's most likely some edge cases but it would work most of the time)
I'd also add a test for that (the current tests do not rename modules)
BenchmarksBenchmark execution time: 2026-05-27 21:51:05 Comparing candidate commit ba9e851 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
What does this PR do?
Makes typing stats comparison follow renames and line moves for RBS declarations and
steep:ignorecomments.Motivation:
Without this change, type reports currently show file renames as "introduced + cleared".
Example:
Change log entry
None.
Additional Notes:
Tests were added for
.github/scripts/typing_stats_compare.rb, for both old and new behavior.To ensure test run, I added it to the same Raketask as our custom Rubocop cops (
spec:custom_cop), but renamed the task tospec:tooling.These tests do not run on
spec:main.How to test the change?
bundle exec rake spec:tooling