-
Notifications
You must be signed in to change notification settings - Fork 187
refactor py_str macro to allow passing custom global/locals #777
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
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 |
| 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"
endThere 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?
| 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_strmacro 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_stris 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.