jkrumbiegel / DataFrameMacros.jl

Macros that simplify working with DataFrames.jl
MIT License
60 stars 5 forks source link

[Bug?] Package doesn't work without `using DataFrames: ByRow` #17

Closed greimel closed 2 years ago

greimel commented 2 years ago

I would like to be explicit about imported names for the convenience of my co-authors. I realized that this works

using DataFrameMacros: @transform
using DataFrames: DataFrame

df = DataFrame(x = rand(100))
@transform(df, :y = @c :x .+ 1)

but this doesn't work

@transform(df, :z = :x + 1)

because ByRow is not defined.

Is there a way to make this work without explicitly using DataFrames: ByRow?

(As an aside, this might be even more confusing, since @c doesn't need to be imported)

jeremiahpslewis commented 2 years ago

Super interesting problem! I looked into it a bit as it seemed like a way to get a better grasp of how the internals of DataFrameMacros works and I can't give any definitive answers, but this seems like it may be a scoping/ namespace issue...

  1. @c looks like it might be sort of a phantom macro which doesn't actually call an @c macro, instead the code just checks whether an @c macro has been specified and then sets a feature flag; line of code which seems relevant is here https://github.com/jkrumbiegel/DataFrameMacros.jl/blob/6b43ab050b870db9ecb92fa5e42d0ecfd54421cc/src/DataFrameMacros.jl#L140

  2. The ByRow call which seems to be problematic is here https://github.com/jkrumbiegel/DataFrameMacros.jl/blob/6b43ab050b870db9ecb92fa5e42d0ecfd54421cc/src/DataFrameMacros.jl#L149 and may be related to the DataFrameMacros expanding your code to include a ByRow function call and then executing the code within the global scope, where ByRow is not available. I wonder this might be resolved by changing to a direct reference to DataFrames.ByRow; just an amateur guess, but I'll try it out and let you know. :)

jeremiahpslewis commented 2 years ago

Seems to work. I'll create a PR.

greimel commented 2 years ago

Thanks, @jeremiahpslewis !

jeremiahpslewis commented 2 years ago

Sure!

jkrumbiegel commented 2 years ago

Whoops that looks like bad macro hygiene on my part, I'll check this soon when I have time