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

Add dependency on MacroTools to avoid Base.remove_linenums! #258

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

pdeffebach
Copy link
Collaborator

After #245, we have some Base.remove_linenums! scattered around. Initially I thought this was harmless, but it was not. See the following

julia> macro linenums_macro(arg)
           if arg isa Expr && arg.head == :block && length(arg.args) == 1 && arg.args[1] isa LineNumberNode
               esc(:(true))
           else
               esc(:(false))
           end
       end
@linenums_macro (macro with 1 method)

On master:

julia> df = DataFrame(a = [1], b = [2]);

julia> @transform(df, y = @linenums_macro begin end)
1×3 DataFrame
 Row │ a      b      y     
     │ Int64  Int64  Bool  
─────┼─────────────────────
   1 │     1      2  false

On this branch

julia> @transform(df, y = @linenums_macro begin end)
1×3 DataFrame
 Row │ a      b      y    
     │ Int64  Int64  Bool 
─────┼────────────────────
   1 │     1      2  true

Basically, macros inside @transform blocks might make assumptions about line number nodes, and we don't want to mess with that. I apologize for using Base.remove_linenums! earlier.

To implement this, I finally bit the bullet and added a dependency on MacroTools.jl. It's just not worth re-inventing the wheel anymore when it comes to these sorts of issues. Every other macro package today has MacroTools as a dependency, so I might do so as well.

We do remove line numbers still, but we only do it when unblock-ing an expression or working with :block-based expression, where as Base.remove_linenums! acts recursively.

I have added a testset so this should be good to merge.

@pdeffebach
Copy link
Collaborator Author

I should ping @nalimilan and @bkamins here to clarify that this is ready for a review.

Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume all is OK. I am not an expert in MacroTools.jl, but I agree with the direction of changes.

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review challenge: only make suggestions on whitespace. :-D

test/dataframes.jl Outdated Show resolved Hide resolved
src/parsing.jl Outdated Show resolved Hide resolved
pdeffebach and others added 3 commits June 21, 2021 13:42
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@pdeffebach pdeffebach merged commit ec66780 into JuliaData:master Jun 21, 2021
@pdeffebach pdeffebach deleted the linenums_fix branch June 21, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants