Skip to content

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Apr 6, 2023

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 (minmax):   422.4 ms456.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 (minmax):   448.1 ms469.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
Copy link
Member Author

MichaReiser commented Apr 6, 2023

@MichaReiser MichaReiser force-pushed the extract-message-emitters branch from 43f9053 to 46de12b Compare April 6, 2023 08:29
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

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 MichaReiser force-pushed the extract-message-emitters branch from 46de12b to 08354a2 Compare April 6, 2023 08:58
use ruff_python_ast::types::Range;

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct Message {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from src/message.rs

grouped_messages
}

pub trait Emitter {
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 is new

@MichaReiser MichaReiser marked this pull request as ready for review April 6, 2023 08:59
@MichaReiser MichaReiser force-pushed the extract-message-emitters branch 2 times, most recently from 74ef23d to d9a3769 Compare April 6, 2023 09:19
@MichaReiser MichaReiser force-pushed the extract-message-emitters branch 2 times, most recently from ab2d3c2 to 6d2ccf7 Compare April 6, 2023 09:42
#[derive(Default)]
pub struct GithubEmitter;

impl Emitter for GithubEmitter {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@MichaReiser MichaReiser Apr 7, 2023

Choose a reason for hiding this comment

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

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

@MichaReiser MichaReiser force-pushed the extract-message-emitters branch from f5193e6 to 2ba3c0d Compare April 11, 2023 07:16
@MichaReiser MichaReiser enabled auto-merge (squash) April 11, 2023 07:21
@MichaReiser MichaReiser merged commit 9209e57 into main Apr 11, 2023
@MichaReiser MichaReiser deleted the extract-message-emitters branch April 11, 2023 07:24
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 11, 2023
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.

3 participants