Refactor expression handling

[pdeffebach]

This PR re-factors the expression handling. As a result it's now simpler, fewer LOC, and has more abstractions so the code is less fragile.

[pdeffebach]

Okay @bkamins, @nalimilan this PR is ready for a review.

Basically, I was tired of having very complicated code for parsing. I also had the column selection methods hard-coded, which resulted in a lot of brittle code.

I also standardized the parsing, so that @eachrow and the main macros have the same parsing rules. There was a small discrepancy in behavior that I added a test for.

Overall I'm happy with this. But I understand this is a hard thing for you to review since it's very technical.

One point to highlight: I use the following pattern

"""
    get_column_expr(x)

If the input is a valid column identifier, i.e.
a `QuoteNode` or an expression beginning with
`$DOLLAR`, returns the underlying identifier.

If input is not a valid column identifier,
returns nothing.
"""
get_column_expr(x) = nothing
function get_column_expr(e::Expr)
    e.head == :$ && return e.args[1]
    onearg(e, :cols) && return e.args[2]
    return nothing
end
get_column_expr(x::QuoteNode) = x

To check if something is a column expression, I check if the output is === nothing. This means I don't have to have two separate code paths -- one for checking if an expression is a column and another for actually getting the column reference. I would assume that the === nothing stuff propagates perfectly so there is no performance loss, as this is how iteration works.

[bkamins]

onearg(e, :cols) && return e.args[2]

is cols deprecated or it will be kept as an alternative to $?

[bkamins]

Yes - it is hard to follow as the changes are small and scattered. But it is nice that all tests pass 😄.

What I would recommend. Can you please add a testset testing get_column_expr explicitly? Then feed into it various values - good and bad (and try to think of some corner cases). This will:

1. make it easier to make sure all is right (and maybe some hard corner cases would come to my mind)
2. kind-of document the expected behavior via tests (so it is easier to understand what this function achieves)

[pdeffebach]

is cols deprecated or it will be kept as an alternative to $?

No, I need to add back in warning messages.

Okay, I will add a test set just for parsing.

[pdeffebach]

Okay I have added new tests that should give a clearer idea of the rules for expression handling.

Ready for a review!

One thing: The functions replace_syms! and eachrow_replace are very similar. However they are different enough that writing a single function would require some over-engineering at this point in time.

[bkamins]

so without $ only symbol is accepted? What is the result of:

@protect Symbol("x")

?

[pdeffebach]

That will give a Symbol, you can't get a column through Symbol("x"). I will add this to the tests to clarify.

[nalimilan]

Just a few minor comments.

[nalimilan]

Doesn't this work?

Suggested change

	`$DOLLAR`, returns the underlying identifier.
	`\$`, returns the underlying identifier.

[pdeffebach]

No, it shows up as a backslash inside the auto-generated docs. This was the only solution that seemed to not cause runtime errors from interpolation and also not cause problems in the html of the docs.

[pdeffebach]

Okay, will merge this after CI and then turn to the AsTable stuff

[pdeffebach]

Actually, I will wait for an approval. With a single approval I will merge.

[pdeffebach]

Thanks!