-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
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? |
IPython Escape Commands TerminologyBefore we get into the definitions, I'm omitting all of the rules of lexing/parsing because we're only interested in the definition.
What the syntax would look like?
Motivation behind the rename:
|
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 A few thoughts:
That makes sense now. So only the EscapeKind 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 |
Yes, will do so in a separate PR.
Yes, my "Other ideas" section proposed something similar although a bit verbose. I like the "Ipy" prefix. So, to summarize:
|
Sounds good to me.
I prefer the |
65a5eb5
to
1283495
Compare
154d16c
to
72ade72
Compare
There was a problem hiding this 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
4ca3014
to
9207a40
Compare
Thanks for noticing the remaining renames! :) |
## 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.
Summary
This PR renames the
MagicCommand
token toIpyEscapeCommand
token andMagicKind
toIpyEscapeKind
type to better reflect the purpose of the token and type. Similarly, it renames the AST nodes fromLineMagic
toIpyEscapeCommand
prefixed withStmt
/Expr
wherever necessary.It also makes renames from using
jupyter_magic
toipython_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:
%
/%%
(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
forjupyter_escape
/magic
to make sure all renames are done correctly.