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

Cols to dollar #266

Merged
merged 20 commits into from
Jul 24, 2021
Merged

Cols to dollar #266

merged 20 commits into from
Jul 24, 2021

Conversation

pdeffebach
Copy link
Collaborator

With this PR, we no longer have @transform df :y = f(cols("x")), but rather @transform df :y = f($"x").

@pdeffebach
Copy link
Collaborator Author

Okay tests pass, so this is ready for a review.

There are two problems that I am fine resolving later, after this is merged.

  1. For the purposes of tests, we still need cols occasionally, particularly in the function_compilation tests. Also, the @linq tests seem to error with $ inside a @testset. Overall, this is strong evidence we need to keep cols around permanently. If users want to use DataFramesMeta.jl macros inside other macros, cols is a great way to get around the mine-field that is interpolation with $ and it's many meanings.
  2. Broadcasting. In general, you can write
x = rand(10); 
@. x - $mean(x)

But you can't do the same in DataFramesMeta.jl. @. x - ^($)mean(:x) does not even work.

This is not a big deal, I guess. You can't have $ work for every instance. But it's a bit of a bummer.

We should still merge, though.

@bkamins
Copy link
Member

bkamins commented Jul 19, 2021

Do you know if the same issues with $ are in DataFrameMacros.jl?

@pdeffebach
Copy link
Collaborator Author

The same issues exist with DataFrameMacros.jl. I've spoken to @jkrumbiegel and he's fine with not working with every use-case of $ out there. We should probably accept that, too, given how many different meanings it has throughout the ecosystem.

It's probably something we can iterate on at a later date, possibly through nesting $$.

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.

Looks good, just a minor comment. The issues you mentioned are annoying, it would be nice to find a way to escape/protect $ from other macros so that we don't have to keep cols forever.

src/eachrow.jl Outdated Show resolved Hide resolved
src/parsing.jl Outdated Show resolved Hide resolved
src/parsing.jl Outdated Show resolved Hide resolved
src/parsing.jl Outdated Show resolved Hide resolved
src/parsing.jl Outdated Show resolved Hide resolved
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@pdeffebach
Copy link
Collaborator Author

Yes. I don't have any ideas on how to do that at the moment, but I will ask people who are more experienced in metaprogramming. Maybe John Myles White can return to his roots and help DataFramesMeta.jl again.

@nalimilan
Copy link
Member

Maybe John Myles White can return to his roots and help DataFramesMeta.jl again.

Actually John hasn't contributed to this package at all. It was created by Tom Short.

@pdeffebach
Copy link
Collaborator Author

Ah sorry! I got my packages confused.

I'm going to merge! This will create merge conflicts I will have to sort out in #267

@pdeffebach pdeffebach merged commit cad62fd into JuliaData:master Jul 24, 2021
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