Skip to content

Remove len from JoinCommaSeparatedBuilder #6185

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 31, 2023
Merged

Remove len from JoinCommaSeparatedBuilder #6185

merged 1 commit into from
Jul 31, 2023

Conversation

MichaReiser
Copy link
Member

Summary

Replaces the len on JoinCommaSeparated by a state enum that tracks whether there's no previous node, its the first node, or any subsequent node.

Test Plan

cargo test

@MichaReiser
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

konstin
konstin approved these changes Jul 31, 2023
View reviewed changes
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.

You do like your enums and accessor methods :D

crates/ruff_python_formatter/src/builders.rs Outdated
/// First entry, ending at the given position.
First(TextSize),
/// Second or any subsequent entry ending at the specific position.
Rest(TextSize),
Copy link
Member

Choose a reason for hiding this comment

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

nit: Call this AtLeastTwo, Rest makes it sound like a head/rest situation

crates/ruff_python_formatter/src/builders.rs Outdated
@@ -245,12 +275,11 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> {
Separator: Format<PyFormatContext<'ast>>,
{
self.result = self.result.and_then(|_| {
if self.end_of_last_entry.is_some() {
if !self.last_entry.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: invert and call has_last_element

@github-actions
Copy link
Contributor

github-actions bot commented Jul 31, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.5±0.09ms     4.8 MB/sec    1.03      8.7±0.17ms     4.7 MB/sec
formatter/numpy/ctypeslib.py               1.00  1683.1±43.79µs     9.9 MB/sec    1.00  1683.0±38.29µs     9.9 MB/sec
formatter/numpy/globals.py                 1.00    183.4±4.43µs    16.1 MB/sec    1.00    182.8±2.09µs    16.1 MB/sec
formatter/pydantic/types.py                1.01      3.6±0.11ms     7.0 MB/sec    1.00      3.6±0.06ms     7.1 MB/sec
linter/all-rules/large/dataset.py          1.01     11.3±0.08ms     3.6 MB/sec    1.00     11.2±0.10ms     3.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.02ms     5.9 MB/sec    1.00      2.8±0.03ms     5.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    382.0±0.79µs     7.7 MB/sec    1.00    383.3±1.41µs     7.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.0±0.05ms     5.1 MB/sec    1.00      5.0±0.03ms     5.1 MB/sec
linter/default-rules/large/dataset.py      1.00      6.0±0.04ms     6.8 MB/sec    1.00      6.0±0.02ms     6.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1239.2±4.81µs    13.4 MB/sec    1.00   1243.1±3.08µs    13.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    135.5±0.72µs    21.8 MB/sec    1.00    136.1±0.84µs    21.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.6±0.01ms     9.8 MB/sec    1.01      2.6±0.01ms     9.7 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.2±0.17ms     4.0 MB/sec    1.00     10.1±0.12ms     4.0 MB/sec
formatter/numpy/ctypeslib.py               1.00  1941.7±23.13µs     8.6 MB/sec    1.02  1971.3±41.69µs     8.4 MB/sec
formatter/numpy/globals.py                 1.00    212.6±4.28µs    13.9 MB/sec    1.03    219.6±6.50µs    13.4 MB/sec
formatter/pydantic/types.py                1.00      4.3±0.10ms     5.9 MB/sec    1.00      4.3±0.08ms     5.9 MB/sec
linter/all-rules/large/dataset.py          1.02     13.7±1.09ms     3.0 MB/sec    1.00     13.5±0.18ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.03ms     4.7 MB/sec    1.00      3.6±0.07ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.01    435.7±7.06µs     6.8 MB/sec    1.00    433.4±9.25µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.1±0.07ms     4.2 MB/sec    1.00      6.1±0.13ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.01      7.6±0.08ms     5.4 MB/sec    1.00      7.5±0.08ms     5.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1521.9±16.74µs    10.9 MB/sec    1.00  1526.6±24.24µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    169.1±3.93µs    17.4 MB/sec    1.00    168.4±6.78µs    17.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.3±0.05ms     7.8 MB/sec    1.00      3.3±0.06ms     7.8 MB/sec

@MichaReiser
Copy link
Member Author

MichaReiser commented Jul 31, 2023 •
edited
Loading

You do like your enums and accessor methods :D

Ohhh I do love them

@MichaReiser MichaReiser force-pushed the remove-len-from-comma-separated-builder branch from c85b721 to e0e75a1 Compare July 31, 2023 12:10
@MichaReiser MichaReiser enabled auto-merge (squash) July 31, 2023 12:10
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Jul 31, 2023
@MichaReiser MichaReiser merged commit 311a1f9 into main Jul 31, 2023
@MichaReiser MichaReiser deleted the remove-len-from-comma-separated-builder branch July 31, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konstin konstin konstin approved these changes

Assignees
No one assigned
Labels
internal An internal refactor or improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MichaReiser @konstin