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

Rename Magic* to IpyEscape* #6395

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Rename Magic* to IpyEscape* #6395

merged 3 commits into from
Aug 9, 2023

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Aug 7, 2023

Summary

This PR renames the MagicCommand token to IpyEscapeCommand token and MagicKind to IpyEscapeKind type to better reflect the purpose of the token and type. Similarly, it renames the AST nodes from LineMagic to IpyEscapeCommand prefixed with Stmt/Expr wherever necessary.

It also makes renames from using jupyter_magic to ipython_escape_commands in various function names.

The mode value is still Mode::Jupyter because the escape commands are part of the IPython syntax but the lexing/parsing is done for a Jupyter notebook.

Motivation behind the rename:

Test Plan

  • cargo test to make sure all renames are done correctly.
  • grep for jupyter_escape/magic to make sure all renames are done correctly.

@dhruvmanila
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      8.4±0.17ms     4.9 MB/sec    1.00      8.2±0.04ms     4.9 MB/sec
formatter/numpy/ctypeslib.py               1.01  1679.0±54.62µs     9.9 MB/sec    1.00  1656.7±52.19µs    10.1 MB/sec
formatter/numpy/globals.py                 1.01    187.4±7.11µs    15.7 MB/sec    1.00    185.5±5.65µs    15.9 MB/sec
formatter/pydantic/types.py                1.00      3.5±0.11ms     7.2 MB/sec    1.00      3.5±0.10ms     7.2 MB/sec
linter/all-rules/large/dataset.py          1.00     10.3±0.09ms     4.0 MB/sec    1.00     10.3±0.06ms     4.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.7±0.02ms     6.1 MB/sec    1.00      2.7±0.01ms     6.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    387.0±2.75µs     7.6 MB/sec    1.00    386.0±1.85µs     7.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.4±0.04ms     4.7 MB/sec    1.00      5.4±0.06ms     4.7 MB/sec
linter/default-rules/large/dataset.py      1.00      5.3±0.02ms     7.7 MB/sec    1.00      5.3±0.02ms     7.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1155.2±8.11µs    14.4 MB/sec    1.00   1143.4±8.02µs    14.6 MB/sec
linter/default-rules/numpy/globals.py      1.02    134.8±4.93µs    21.9 MB/sec    1.00    132.7±3.98µs    22.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.4±0.03ms    10.6 MB/sec    1.00      2.4±0.01ms    10.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     12.0±0.30ms     3.4 MB/sec    1.03     12.3±0.28ms     3.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.3±0.07ms     7.4 MB/sec    1.04      2.4±0.08ms     7.1 MB/sec
formatter/numpy/globals.py                 1.00   258.9±12.15µs    11.4 MB/sec    1.02   264.3±21.92µs    11.2 MB/sec
formatter/pydantic/types.py                1.00      5.1±0.15ms     5.0 MB/sec    1.02      5.2±0.17ms     4.9 MB/sec
linter/all-rules/large/dataset.py          1.02     15.3±0.36ms     2.7 MB/sec    1.00     15.1±0.31ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      4.2±0.14ms     3.9 MB/sec    1.00      4.2±0.09ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   517.2±18.56µs     5.7 MB/sec    1.01   520.1±22.17µs     5.7 MB/sec
linter/all-rules/pydantic/types.py         1.01      7.9±0.19ms     3.2 MB/sec    1.00      7.9±0.13ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.00      8.3±0.12ms     4.9 MB/sec    1.00      8.3±0.12ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1716.9±40.04µs     9.7 MB/sec    1.02  1752.6±47.05µs     9.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    197.0±7.68µs    15.0 MB/sec    1.03    203.2±8.21µs    14.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.08ms     7.0 MB/sec    1.02      3.7±0.07ms     6.9 MB/sec

@MichaReiser
Copy link
Member

You mention that the rename is to better reflect the purpose and type. Can you expand on what makes the new name the better choice?

@dhruvmanila
Copy link
Member Author

IPython Escape Commands Terminology

Before we get into the definitions, I'm omitting all of the rules of lexing/parsing because we're only interested in the definition.

  • "Magic commands" are special IPython syntax which starts with a token to identify the magic kind followed by the command value itself.
  • "Magic kind" are the kind of magic commands which are recognized by the token itself: %, %%, !, !!, ?, ??, /, ;, and ,.
  • "Help command" (or "Dynamic Object Introspection" as it's called) are the escape commands of the kind ? and ??. (?str.replace)
  • "Help end command" are a subset of "Help command" where the token can be at the end of the line i.e., after the value itself (str.replace?). Here's where things get tricky. I'll divide the help end command into two types for better understanding:
    • Strict version: The token is only at the end of the line (str.replace?, str.replace??)
    • Combined version: Not only either ? or ?? is at the end of the line but other magic kind tokens are present at the start as well (%matplotlib?, %%timeit?)
  • "Priority" comes into picture for the "Combined version" mentioned above. When there are magic kind tokens on both side of the value, how do we determine the magic kind i.e., which token to choose? The "Help end command" always takes priority over any other token i.e., if there is ?/?? at the end then that is used to determine the kind.

What the syntax would look like?

  • <MagicKind><Command value> (e.g., %matplotlib, /foo, !pwd, etc.)
  • <Command value><MagicKind ("?" or "??")> (e.g., str.replace?, math.pi??, etc.)
  • <MagicKind><Command value><MagicKind ("?" or "??")> (e.g., %matplotlib?, %%timeit??, etc.)

Motivation behind the rename:

@MichaReiser
Copy link
Member

Thanks for the explanation. This is very helpful context that I didn't have before. We should preserve this in writing somewhere (maybe as documentation on EscapeCommand).

A few thoughts:

The word "magic" is used mainly for the actual magic commands i.e., the ones starting with %/%%

That makes sense now. So only the EscapeKind % and %% are "magic"

What I liked about the old "Magic" prefix is that it clearly marked them as not being part of the Python spec. Would it make sense to keep a similar prefix. Maybe IpyEscapeCommand and IpyEscapeSequence. Should we use the same prefix for the tokens and ast nodes?

We did something similar at rome where we prefixed the JS Spec nodes with Js, e.g. JsBinaryExpression, the type script syntax nodes with Ts (TsTypeAliasDeclaration), and the jsx nodes with Jsx (JsxTagExpression)

@dhruvmanila
Copy link
Member Author

Thanks for the explanation. This is very helpful context that I didn't have before. We should preserve this in writing somewhere (maybe as documentation on EscapeCommand).

Yes, will do so in a separate PR.

What I liked about the old "Magic" prefix is that it clearly marked them as not being part of the Python spec. Would it make sense to keep a similar prefix. Maybe IpyEscapeCommand and IpyEscapeSequence. Should we use the same prefix for the tokens and ast nodes?

Yes, my "Other ideas" section proposed something similar although a bit verbose. I like the "Ipy" prefix.

So, to summarize:

  • MagicCommand -> IpyEscapeCommand
  • MagicKind -> IpyEscapeKind (I'm leaning towards "Kind" suffix for consistency with StringKind although I have no problem with IpyEscapeSequence)
  • StmtLineMagic/ExprLineMagic -> IpyEscapeCommandStatement/IpyEscapeCommandExpression (Is this too verbose?) or StmtIpyEscapeCommand/ExprIpyEscapeCommand (for consistency with the current naming convention).

@MichaReiser
Copy link
Member

Sounds good to me.

StmtLineMagic/ExprLineMagic -> IpyEscapeCommandStatement/IpyEscapeCommandExpression (Is this too verbose?) or StmtIpyEscapeCommand/ExprIpyEscapeCommand (for consistency with the current naming convention).

I prefer the IpyEscapeCommandStatement but we may need to go with StmtIpyEscapeCommand for consistency (until we rename all nodes)

@dhruvmanila dhruvmanila changed the title Rename Magic* to Escape* Rename Magic* to IpyEscape* Aug 9, 2023
@dhruvmanila dhruvmanila added the internal An internal refactor or improvement label Aug 9, 2023
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good. You may want to search for StmtLineMagic and ExprLineMagic (text) and replace all occurrences with the new names

crates/ruff_python_ast/src/node.rs Outdated Show resolved Hide resolved
crates/ruff_python_ast/src/node.rs Outdated Show resolved Hide resolved
crates/ruff_python_ast/src/comparable.rs Outdated Show resolved Hide resolved
@dhruvmanila
Copy link
Member Author

Thanks for noticing the remaining renames! :)

@dhruvmanila dhruvmanila enabled auto-merge (squash) August 9, 2023 13:20
@dhruvmanila dhruvmanila merged commit 6a64f22 into main Aug 9, 2023
16 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/ipython-magics-rename branch August 9, 2023 13:28
durumu pushed a commit to durumu/ruff that referenced this pull request Aug 12, 2023
## Summary

This PR renames the `MagicCommand` token to `IpyEscapeCommand` token and
`MagicKind` to `IpyEscapeKind` type to better reflect the purpose of the
token and type. Similarly, it renames the AST nodes from `LineMagic` to
`IpyEscapeCommand` prefixed with `Stmt`/`Expr` wherever necessary.

It also makes renames from using `jupyter_magic` to
`ipython_escape_commands` in various function names.

The mode value is still `Mode::Jupyter` because the escape commands are
part of the IPython syntax but the lexing/parsing is done for a Jupyter
notebook.

### Motivation behind the rename:
* IPython codebase defines it as "EscapeCommand" / "Escape Sequences":
* Escape Sequences:
https://github.com/ipython/ipython/blob/292e3a23459ca965b8c1bfe2c3707044c510209a/IPython/core/inputtransformer2.py#L329-L333
* Escape command:
https://github.com/ipython/ipython/blob/292e3a23459ca965b8c1bfe2c3707044c510209a/IPython/core/inputtransformer2.py#L410-L411
* The word "magic" is used mainly for the actual magic commands i.e.,
the ones starting with `%`/`%%`
(https://ipython.readthedocs.io/en/stable/interactive/reference.html#magic-command-system).
So, this avoids any confusion between the Magic token (`%`, `%%`) and
the escape command itself.
## Test Plan

* `cargo test` to make sure all renames are done correctly.
* `grep` for `jupyter_escape`/`magic` to make sure all renames are done
correctly.
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.

2 participants