-
Notifications
You must be signed in to change notification settings - Fork 189
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
refactor py_str macro to allow passing custom global/locals #777
base: master
Are you sure you want to change the base?
Conversation
Could you clarify why this should be a macro? |
src/pyeval.jl
Outdated
esc(macroexpand(__module__, :(PyCall.@_py_str($m, $m, $code, $(options...))))) | ||
end | ||
|
||
macro _py_str(pyglobals, pylocals, code, options...) |
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.
If we are going to use a function for this, it should take a named tuple (__module__=__module__, __source__=__source__)
as the first argument so that all the information available within a usual macro is available. I think it's better to make it a single first argument rather than two arguments because future versions of julia
may introduce some more information within a macro definition but it'd be bad to change the calling convention of this function.
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.
Also, we should use options
rather than options...
.
So it stems from the fact that inside Given that you want to That said, I realize now all the |
@@ -202,32 +198,37 @@ pasted into the Python code. This allows you to evaluate code | |||
where the code itself is generated by a Julia expression. | |||
""" | |||
macro py_str(code, options...) | |||
m = :(pynamespace($__module__)) | |||
fname = String(__source__.file) | |||
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...)))) |
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.
Why is this call to esc
necessary, since there are esc
calls within @_py_str
?
Indeed, the call to esc
here seems like it might cause problems, because it prevents the names pylocals
, pyglobals
, and ret
from being hygienized.
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.
No, pylocals
etc... get hygenized by @_py_str
, the esc
here makes it so the interpolated variables don't also get hygenized, e.g. with the code in this PR:
julia> @macroexpand py"1+$i"
quote
#= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:229 =#
(var"#18#pyglobals", var"#19#pylocals") = (PyCall.pynamespace(Main), PyCall.pynamespace(Main))
#= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:230 =#
begin
var"#19#pylocals"["__julia_localvar_2_1"] = PyCall.PyObject(i)
end
#= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:231 =#
var"#20#ret" = (PyAny)(PyCall.pyeval_((string)("1+__julia_localvar_2_1"), var"#18#pyglobals", var"#19#pylocals", 258, "none"))
#= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:232 =#
begin
PyCall.delete!(var"#19#pylocals", "__julia_localvar_2_1")
end
#= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:233 =#
var"#20#ret"
end
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.
Maybe I should've asked this before but why is insert_pyevals
run at run-time? It looks like this doesn't depend on the run-time value of globals
and locals
(or maybe I'm missing something)? If we expand everything at macro-expansion time, perhaps esc
of @py_str
here and @prepare_for_pyjulia_call
there can be removed? Then maybe _py_str
doesn't have to be a macro?
@@ -202,32 +198,37 @@ pasted into the Python code. This allows you to evaluate code | |||
where the code itself is generated by a Julia expression. | |||
""" | |||
macro py_str(code, options...) | |||
m = :(pynamespace($__module__)) | |||
fname = String(__source__.file) | |||
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...)))) |
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.
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...)))) | |
esc(:($PyCall.@_py_str($m, $m, $fname, $code, $(options...)))) |
I think you need this?
I think we better test py""
with something like
baremodule TestPyStr
using PyCall: @py_str
pystr(x) = py"$$x"
end
@test TestPyStr.pystr("1+1") == 2
This should have no change to functionality, but the new
@_py_str
macro can be used by other functions to pass custom globals/locals dicts into Python expression while still getting the benefit of@py_str
's variable interpolation.@_py_str
is kept a macro rather than function because it makes downstream macro hygene a bit easier.I'm using this here JuliaPy/pyjulia#381, where this can maybe be discussed more.