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

Don't expand user variables in execution magics #11516

Merged
merged 2 commits into from Dec 6, 2018

Conversation

@minrk
Copy link
Member

commented Nov 29, 2018

Adds @magic.no_var_expand decorator to mark a magic as opting out of variable expansion.

Most useful for magics that take Python code on the line, e.g. %timeit, etc.

I applied it to the execution magics (timeit, time, debug, prun). I'm not sure if there are other magics that should get this treatment.

minrk added some commits Nov 29, 2018

Don't expand user variables in execution magics
Adds magic.no_var_expand decorator to mark a magic as opting out of variable expansion

Most useful for magics that take Python code on the line, e.g. %timeit, etc.
@Carreau

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

That's sounds reasonable to me. Does it has any reason to break user code ?

@Carreau Carreau added this to the 7.2 milestone Nov 29, 2018

@Carreau

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

I'm going to leave that for 7.3 I'm worried about the consequences and would like it on master for a little longer. I'm going to do a 7.2 today. Hope that's ok with you.

@Carreau Carreau removed this from the 7.2 milestone Nov 30, 2018

@minrk

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

Does it has any reason to break user code ?

I don't think so, but you never know. The one valid use case for variable expansion that I can think of is shell-style args, leaving the code block unparsed. e.g. this technically makes sense:

dest = "myresult"
%timeit -o $dest some_python_code()
print(myresult)

But with this patch (and the magics implementation in general) variable expansion is all or nothing. We could try a deeper fix to try to only disable parsing of the code block, but that would require pretty deep changes, as the decision to implement var expansion would need to happen at a level where arguments are understood, violating the "magics are passed the line as a string" concept.

Another technically valid use could be building the code to time in a Python variable, and then running:

code = "np.sin(x)"
%timeit {code}

I've never seen it before, but it might exist in the wild. In both cases, I think the tradeoff of never mangling the Python code is worth it.

I'm going to leave that for 7.3

👍 I had no intention of sneaking this under the wire, I just hadn't noticed that 7.2 was ready for release. 7.3 is perfect.

@Carreau Carreau added this to the 7.3 milestone Dec 6, 2018

@Carreau

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Sounds good, let's get that in.

@Carreau Carreau merged commit a13d482 into ipython:master Dec 6, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 68.36% (+0.01%) compared to 78441b6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.