Extract message emitters from Printer by MichaReiser · Pull Request #3895 · astral-sh/ruff

MichaReiser · 2023-04-06T08:02:47Z

This PR extracts the emitters for serializing Messages from the Printer in the CLI to ruff. The reason for moving the implementations to ruff is so that I use the text emitter to write the linter snapshots instead of serializing the Diagnostic to YAML. Using the rendered text output has the advantage that we can do refactors like replacing Location with TextSize without affecting the snapshot outputs, which gives us confidence in the refactor.

I used this chance to:

Add tests for all emitters
Rewrite some emitters to use write directly rather than using format and then writing the formatted string.

Considerations

Introducing a shared trait for the emitters isn't necessary from an implementation point of view because the Printer calls emit statically on all emitters (it isn't using dynamic dispatching). However, it helped to share logic between tests and it ensures that all emitters are structured similarly.
Implementing all emitters in the ruff crate isn't strictly necessary. For example, I don't plan to use the junit emitter for the test infrastructure. I still decided to put them all into the same module for better discoverability.

Performance

Avoiding format and to_string improves performance a bit

hyperfine --warmup 10 \
          "./target/release/ruff ./crates/ruff/resources/test/cpython/ -e --select=ALL" \
          "./ruff-main ./crates/ruff/resources/test/cpython/ -e --select=ALL"
Benchmark 1: ./target/release/ruff ./crates/ruff/resources/test/cpython/ -e --select=ALL
  Time (mean ± σ):     442.6 ms ±  10.0 ms    [User: 722.9 ms, System: 509.2 ms]
  Range (min … max):   422.4 ms … 456.9 ms    10 runs
 
Benchmark 2: ./ruff-main ./crates/ruff/resources/test/cpython/ -e --select=ALL
  Time (mean ± σ):     456.9 ms ±   7.0 ms    [User: 724.0 ms, System: 507.0 ms]
  Range (min … max):   448.1 ms … 469.2 ms    10 runs
 
Summary
  './target/release/ruff ./crates/ruff/resources/test/cpython/ -e --select=ALL' ran
    1.03 ± 0.03 times faster than './ruff-main ./crates/ruff/resources/test/cpython/ -e --select=ALL'

MichaReiser · 2023-04-06T08:02:59Z

Current dependencies on/for this PR:

main
- PR Extract message emitters from Printer #3895 👈
  - PR Cheap cloneable LineIndex #3896
    - PR Store source code on message #3897
      - PR Render code frame with context #3901
        
        PR Introduce SourceFile to avoid cloning the message filename #3904
        
        PR Order Edits by Locations #3905
        
        PR Pretty print Diagnostics in snapshot tests #3906

This comment was auto-generated by Graphite.

github-actions · 2023-04-06T08:52:44Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     21.0±0.59ms  1987.7 KB/sec    1.00     20.9±0.65ms  1994.9 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      5.1±0.18ms     3.3 MB/sec    1.00      5.0±0.19ms     3.3 MB/sec
linter/all-rules/numpy/globals.py          1.02   639.2±27.48µs     4.6 MB/sec    1.00   627.7±22.28µs     4.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      8.7±0.32ms     2.9 MB/sec    1.01      8.8±0.36ms     2.9 MB/sec
linter/default-rules/large/dataset.py      1.01     10.4±0.51ms     3.9 MB/sec    1.00     10.3±0.31ms     3.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00      2.2±0.07ms     7.4 MB/sec    1.00      2.2±0.08ms     7.4 MB/sec
linter/default-rules/numpy/globals.py      1.01   271.0±12.66µs    10.9 MB/sec    1.00   267.6±13.81µs    11.0 MB/sec
linter/default-rules/pydantic/types.py     1.02      4.8±0.26ms     5.3 MB/sec    1.00      4.7±0.18ms     5.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     22.9±1.10ms  1822.3 KB/sec    1.00     22.5±1.49ms  1850.1 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      6.2±0.49ms     2.7 MB/sec    1.00      6.0±0.46ms     2.8 MB/sec
linter/all-rules/numpy/globals.py          1.01   681.5±64.91µs     4.3 MB/sec    1.00   672.4±41.13µs     4.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      9.6±0.68ms     2.7 MB/sec    1.03      9.9±0.65ms     2.6 MB/sec
linter/default-rules/large/dataset.py      1.00     11.0±0.86ms     3.7 MB/sec    1.06     11.6±0.60ms     3.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00      2.3±0.15ms     7.3 MB/sec    1.09      2.5±0.11ms     6.7 MB/sec
linter/default-rules/numpy/globals.py      1.00   286.6±22.19µs    10.3 MB/sec    1.05   302.1±24.35µs     9.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      5.0±0.38ms     5.1 MB/sec    1.08      5.4±0.58ms     4.7 MB/sec

MichaReiser · 2023-04-06T08:31:25Z

crates/ruff/src/message/mod.rs

+use ruff_python_ast::types::Range;
+
+#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
+pub struct Message {


Copied from src/message.rs

MichaReiser · 2023-04-06T08:31:32Z

crates/ruff/src/message/mod.rs

+    grouped_messages
+}
+
+pub trait Emitter {


This is new

charliermarsh · 2023-04-06T22:05:53Z

crates/ruff/src/message/github.rs

+#[derive(Default)]
+pub struct GithubEmitter;
+
+impl Emitter for GithubEmitter {


My read on the Rust naming rules is that this is correct (over GitHubEmitter).

I don't know. I chose this naming to keep it consistent with SerializationFormat::Github

MichaReiser force-pushed the extract-message-emitters branch from 43f9053 to 46de12b Compare April 6, 2023 08:29

MichaReiser force-pushed the extract-message-emitters branch from 46de12b to 08354a2 Compare April 6, 2023 08:58

MichaReiser commented Apr 6, 2023

View reviewed changes

MichaReiser requested a review from charliermarsh April 6, 2023 08:58

MichaReiser marked this pull request as ready for review April 6, 2023 08:59

MichaReiser force-pushed the extract-message-emitters branch 2 times, most recently from 74ef23d to d9a3769 Compare April 6, 2023 09:19

MichaReiser mentioned this pull request Apr 6, 2023

Cheap cloneable LineIndex #3896

Merged

MichaReiser force-pushed the extract-message-emitters branch 2 times, most recently from ab2d3c2 to 6d2ccf7 Compare April 6, 2023 09:42

This was referenced Apr 6, 2023

Store source code on message #3897

Merged

Render code frame with context #3901

Merged

charliermarsh reviewed Apr 6, 2023

View reviewed changes

charliermarsh approved these changes Apr 6, 2023

View reviewed changes

This was referenced Apr 7, 2023

Introduce SourceFile to avoid cloning the message filename #3904

Merged

Order Edits by Locations #3905

Merged

Pretty print Diagnostics in snapshot tests #3906

Merged

MichaReiser added 2 commits April 11, 2023 09:13

Avoid allocating a new string when printing annotations

78c2523

Fix color handling in tests

2ba3c0d

MichaReiser force-pushed the extract-message-emitters branch from f5193e6 to 2ba3c0d Compare April 11, 2023 07:16

MichaReiser enabled auto-merge (squash) April 11, 2023 07:21

MichaReiser merged commit 9209e57 into main Apr 11, 2023

MichaReiser deleted the extract-message-emitters branch April 11, 2023 07:24

MichaReiser added the internal An internal refactor or improvement label Apr 11, 2023

ntBre mentioned this pull request Jul 10, 2025

Render Azure, JSON, and JSON lines output with the new diagnostics #19133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Extract message emitters from Printer#3895

Extract message emitters from Printer#3895
MichaReiser merged 2 commits intomainfrom
extract-message-emitters

MichaReiser commented Apr 6, 2023 •

edited

Loading

Uh oh!

MichaReiser commented Apr 6, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Apr 6, 2023 •

edited

Loading

Uh oh!

MichaReiser Apr 6, 2023

Uh oh!

MichaReiser Apr 6, 2023

Uh oh!

charliermarsh Apr 6, 2023

Uh oh!

MichaReiser Apr 7, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

MichaReiser commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Considerations

Performance

Uh oh!

MichaReiser commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

MichaReiser Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MichaReiser commented Apr 6, 2023 •

edited

Loading

MichaReiser commented Apr 6, 2023 •

edited

Loading

github-actions bot commented Apr 6, 2023 •

edited

Loading

MichaReiser Apr 7, 2023 •

edited

Loading