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

Recurse _ to bypass square brackets, and usually-infix operators #10

Merged
merged 9 commits into from Jul 8, 2020

Conversation

mcabbott
Copy link
Collaborator

Ref #8.

There may be weird edge cases I've missed, for instance what happens if _ appears inside [ ]?

Copy link
Owner

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Thanks very much, looks pretty good to me. I feel like we should try to document this in terms of "parenthesized expressions" or something... to give people the rough idea of intention of the syntax, even though we can't quite guarantee it in all cases? Then put an extended discussion of the caveats in a separate block.

src/Underscores.jl Outdated Show resolved Hide resolved
src/Underscores.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
src/Underscores.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Collaborator Author

mcabbott commented May 25, 2020

Trying to think of edge cases, what should happen to each of these?

julia> @_ [rand(1:3.0, 5) for _ in 1:4] |> Int[sum(_^2, x) for x in __]
ERROR: MethodError: no method matching length(::var"#58#63")

The first _ here is an error by itself, and perhaps should be ignored. The second, in sum(_^2, x), could be made a function if the rule that "_ ignores []" is slightly wider, to include :typed_comprehension etc. Although I suppose you could use a comprehension to make a list of anonymous functions.

julia> @_ rand(4) |> [ sum(__)       sum(_^2, __)
                       sum(_^3, __)  sum(_^4, __) ]
ERROR: syntax: invalid syntax sum(Core.getfield(#self#, :__1)) sum(literal_pow(^, _1, Core.apply_type(Val, 2)()), Core.getfield(#self#, :__1))

Likewise, "_ ignores []" could allow this, or it could return a matrix of functions (if only indexing brackets are ignored), or it could deliberately be an error. (Heads :vcat, :row, etc.)

julia> using AxisKeys # understands functions inside indexing

julia> @_ wrapdims(randn(5), 10:10:50)[_>33] # here `>(33)` works
ERROR: syntax: all-underscore identifier used as rvalue

This last one perhaps is the right error. This is a weird use of :ref.

@mcabbott
Copy link
Collaborator Author

mcabbott commented May 26, 2020

I moved the discussion to the end of Extended help. It's leaving space for discussing operators etc. (Which I can add to this PR if you don't mind not keeping things orthogonal?)

The last commit handles the above comprehensions etc. I'm not entirely sure you will want this, easy to revert.

@c42f
Copy link
Owner

c42f commented May 29, 2020

Sorry a bit swamped!

It's leaving space for discussing operators etc. (Which I can add to this PR if you don't mind not keeping things orthogonal?)

Right, I think it's actually non-orthogonal anyway - in my mind this is very much all one semantic change from "_ closure goes into the outermost-but-one" AST node to "_ goes into the outermost-but-one parenthesized thing".

The last commit handles the above comprehensions etc. I'm not entirely sure you will want this, easy to revert.

I think it's ok, but perhaps this should happen not by exclusion, but by inclusion: have a few things whitelisted to receive _ rather than a bunch of things blacklisted?

@mcabbott
Copy link
Collaborator Author

Sure, no rush.

I added what I had for operators onto this now.

perhaps this should happen not by exclusion, but by inclusion: have a few things whitelisted to receive _ rather than a bunch of things blacklisted?

Do you mean as a matter of documentation, or implementation?

To make the list comprehensions work (under "square brackets") this also recurses past un-collected generators. Which is, as you say, another move towards only treating ordinary function calls.

@mcabbott mcabbott changed the title Recurse _ past indexing [] Recurse _ to bypass square brackets, and usually-infix operators May 29, 2020
src/Underscores.jl Outdated Show resolved Hide resolved
Copy link
Owner

@c42f c42f left a comment

Choose a reason for hiding this comment

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

lower_inner is a good idea, I agree we need something here.


# Matrix construction
@test 4*ones(2,2) == @_ ones(4) |> [ sum(__) sum(_^2, __)
sum(_^3, __) sum(_^4, __) ]
Copy link
Owner

Choose a reason for hiding this comment

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

Haha, yes I guess this does and should work. Though I wouldn't recommend it :-D

@@ -40,6 +54,28 @@ using Test
Map(_.y)

@test [1] == @_(Map(_.y) ∘ Filter(startswith(_.x, "a")))(data)

Copy link
Owner

Choose a reason for hiding this comment

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

Good examples of things which should work 👍

I must admit some of these caught me by surprise, mostly in a good way :-) Taken out of context they look like things one "shouldn't" do for readability (that broadcast example does my head in!) or genericity (eg, accessing .re).

I may add a small note that not all of these things are recommended as isolated examples but that they could make sense in the context of a larger data pipeline.

@c42f
Copy link
Owner

c42f commented Jul 8, 2020

Thanks @mcabbott this is a great improvement. I've been slow enough getting to this that I thought it's important I added you as a collaborator — if you're using it regularly I'd hate for you to get blocked by my tardyness!

I think we could release a new version soon, but I've a sneaking suspicion that there's a few more breaking syntax changes required which we have yet to deal with. So I opened #12 to keep track of the more general problem.

@mcabbott mcabbott deleted the index branch July 8, 2020 07:30
@mcabbott
Copy link
Collaborator Author

mcabbott commented Jul 8, 2020

No problem, it's good to have several eyes on things. And glad you like my weird examples :)

I see now I had one more commit locally about macros, but this may need more discussion anyway, will make a fresh PR.

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.

None yet

2 participants