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

format StmtAsyncWith #5376

Merged
merged 4 commits into from
Jun 28, 2023
Merged

Conversation

davidszotten
Copy link
Contributor

@davidszotten davidszotten commented Jun 26, 2023

refactor FormatStmtWith and share impl with FormatStmtAsyncWith

tests: snapshots

ref: #5368

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      6.7±0.01ms     6.1 MB/sec    1.00      6.7±0.02ms     6.1 MB/sec
formatter/numpy/ctypeslib.py               1.00   1543.8±1.61µs    10.8 MB/sec    1.00   1543.5±7.16µs    10.8 MB/sec
formatter/numpy/globals.py                 1.01    187.9±1.37µs    15.7 MB/sec    1.00    186.9±2.04µs    15.8 MB/sec
formatter/pydantic/types.py                1.00      3.5±0.01ms     7.4 MB/sec    1.00      3.5±0.01ms     7.4 MB/sec
linter/all-rules/large/dataset.py          1.03     13.4±0.03ms     3.0 MB/sec    1.00     13.0±0.02ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      3.4±0.00ms     4.9 MB/sec    1.00      3.3±0.00ms     5.0 MB/sec
linter/all-rules/numpy/globals.py          1.01    430.2±0.50µs     6.9 MB/sec    1.00    427.3±1.68µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.03      6.0±0.01ms     4.3 MB/sec    1.00      5.8±0.01ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.05      7.0±0.01ms     5.8 MB/sec    1.00      6.6±0.01ms     6.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.04   1518.3±2.13µs    11.0 MB/sec    1.00   1465.6±3.04µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.03    168.1±1.79µs    17.6 MB/sec    1.00    163.4±0.49µs    18.1 MB/sec
linter/default-rules/pydantic/types.py     1.05      3.1±0.01ms     8.1 MB/sec    1.00      3.0±0.01ms     8.5 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.8±0.07ms     5.2 MB/sec    1.01      7.9±0.10ms     5.2 MB/sec
formatter/numpy/ctypeslib.py               1.00  1648.8±10.36µs    10.1 MB/sec    1.01   1661.5±9.26µs    10.0 MB/sec
formatter/numpy/globals.py                 1.00    187.9±1.51µs    15.7 MB/sec    1.02    191.3±2.35µs    15.4 MB/sec
formatter/pydantic/types.py                1.00      3.8±0.03ms     6.8 MB/sec    1.00      3.8±0.02ms     6.8 MB/sec
linter/all-rules/large/dataset.py          1.00     14.9±0.26ms     2.7 MB/sec    1.00     14.9±0.22ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.07ms     4.6 MB/sec    1.00      3.6±0.05ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.00    409.4±5.85µs     7.2 MB/sec    1.00    408.6±5.15µs     7.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.4±0.07ms     4.0 MB/sec    1.00      6.4±0.08ms     4.0 MB/sec
linter/default-rules/large/dataset.py      1.00      7.6±0.08ms     5.4 MB/sec    1.00      7.6±0.06ms     5.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1528.6±14.09µs    10.9 MB/sec    1.00   1532.1±9.50µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    167.7±1.35µs    17.6 MB/sec    1.01    169.1±1.69µs    17.5 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.3±0.02ms     7.7 MB/sec    1.00      3.3±0.02ms     7.7 MB/sec

refactor `FormatStmtWith` and share impl with `FormatStmtAsyncWith`
@davidszotten davidszotten marked this pull request as ready for review June 26, 2023 20:07
@konstin konstin added the formatter Related to the formatter label Jun 28, 2023
@MichaReiser MichaReiser enabled auto-merge (squash) June 28, 2023 10:14
@MichaReiser MichaReiser merged commit c7adb91 into astral-sh:main Jun 28, 2023
@davidszotten
Copy link
Contributor Author

could you elaborate on what you mean by "to make ASYNC more explicit"?

(given that commit can do the same for try/trystar)

@MichaReiser
Copy link
Member

Sure.

My main thinking is to use an enum if the implementations for the different types do not need to customize the formatting (significantly). Here, the only difference is the async and we can simply detect this by testing if the enum variant is AsyncWith. The main reason is that the trait doesn't integrate as nicely into the Format infrastructure because the trait cannot override Format::fmt, instead it is necessary to call the custom fmt_<node> method.

@davidszotten
Copy link
Contributor Author

ok. implementing Format is nice (though this approach does add a fair amount of boilerplate, see e.g. https://github.com/astral-sh/ruff/pull/5418/files )

@MichaReiser
Copy link
Member

ok. implementing Format is nice (though this approach does add a fair amount of boilerplate, see e.g. #5418 (files) )

That's true!. Rome uses a declare_node_union! macro because declaring enums over a set of variants turned out to be an extremely valuable pattern. I think we could write something similar in the future to cut down on the boilerplate

https://github.com/rome/tools/blob/38104b339da8ff688f469799e1fc3f4a46f3d2ec/crates/rome_rowan/src/macros.rs#L47-L141

The rome version generates a bit more than what we need here but I found it super cool.

@davidszotten davidszotten deleted the format-stmt-asyncwith branch July 7, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants