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

Simplify macro rewriters and avoid some prospective bugs #238

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

thautwarm
Copy link

You may want to check this clean implementation of @select:
https://github.com/thautwarm/Query.jl/blob/34f6dbaf5aba156fafa9d60335126a7795f6b676/src/table_query_macros.jl#L49

macro select(args...)
    foldl(args, init=NamedTuple()) do prev, arg
        @match arg begin
            :(everywhere()) => :_

            ::Int && if arg > 0 end =>
                :( merge($prev, QueryOperators.NamedTupleUtilities.select(_, Val(keys(_)[$arg]))) )

            ::Int && if arg < 0 end =>
                let sel = ifelse(prev == NamedTuple(), :_, prev)
                    :( QueryOperators.NamedTupleUtilities.remove($sel, Val(keys($sel)[-$arg])) )
                end
            ::QuoteNode =>
                :( merge($prev, QueryOperators.NamedTupleUtilities.select(_, Val($(arg)))) )
     ...
end

Also, there're some prospective bugs that've been avoided, e.g., quoted " in fieldname of dataframes.

df |> @select(startwith("\"a\""))

@thautwarm
Copy link
Author

I have no idea about the compatibility issues when Julia version is lower than v1.0 :(

@davidanthoff
Copy link
Member

I have no idea about the compatibility issues when Julia version is lower than v1.0 :(

I'll look through the rest of the PR when I have bit more time, but just a quick response on this: I think we could probably drop julia 0.7 support at this point. I think we should keep 1.0 support, given that that is the long term supported version of julia.

@thautwarm
Copy link
Author

Perfect, I'd like to continue refactoring those macro rewriters.

@codecov-io
Copy link

codecov-io commented Mar 2, 2019

Codecov Report

Merging #238 into master will increase coverage by 0.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   90.02%   90.75%   +0.73%     
==========================================
  Files           5        5              
  Lines         441      411      -30     
==========================================
- Hits          397      373      -24     
+ Misses         44       38       -6
Impacted Files Coverage Δ
src/Query.jl 78.57% <ø> (ø) ⬆️
src/table_query_macros.jl 100% <100%> (+7.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da5ea3a...28293b0. Read the comment docs.

@thautwarm
Copy link
Author

thautwarm commented Mar 2, 2019

There seems to be some strange issues in Julia v1.0. I cannot solve it on my own so I went to community for help. https://discourse.julialang.org/t/strange-bug-segmentation-fault-11-solely-existed-in-julia-v1-0/21392
Also, I've already made total simplification for table_query_macros.jl, becoming capable of raising an error when users' input is not in expected case, which I think might be what you expect but don't have time to deal with.

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