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 shortcuts for Elements(), Properties(), @optic #125

Merged
merged 4 commits into from
Jan 6, 2024

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Jan 4, 2024

Allows for shorter and much nicer to read:

  • @optic _[∗].a instead of @optic _ |> Elements() |> _.a,
  • @optic _[∗ₚ][2] instead of @optic _ |> Properties() |> _[2].

@jw3126
Copy link
Member

jw3126 commented Jan 5, 2024

I wished several times, that these were easier to type. My main issue is that they require so many pipes and underscores around them. I wonder if we could do something like @optic _.@Elements().myfield. Possibly with a shorter name than @Elements. Or another mechanism than @ that makes chaining easier to type, I don't really like to collide with valid getindex usage.
I also wonder if we should have @o as a shorthand for @optic, I think I saw that already in one of your packages.

One downside of the current proposal is that it collides with getindex.

@jw3126
Copy link
Member

jw3126 commented Jan 5, 2024

btw thanks a lot for all the work you put into accessors already this year 😄

@aplavin
Copy link
Member Author

aplavin commented Jan 5, 2024

My main issue is that they require so many pipes and underscores around them.

Same with me. These shortcuts (and @o for optic as well) are included in AccessorsExtra, and turned out to be the features I use the most :) And miss the most, when using Accessors alone.

One downside of the current proposal is that it collides with getindex.

Especially for Elements, getindex seems quite natural visually: xxx[∗]. What exactly is the "collision" you have in mind? Note that (\ast) is not *, so it's not type piracy! It's a unicode shortcut, I think it's fine to have unicode symbols as shortcuts as long as it's not the only way.

Alternatively, @optic _ |> ∗ |> _.a could be the intermediate approach – but unfortunately requires parentheses here.

That's why I went with getindex: it's short, convenient, doesn't require extra chaining syntax, and basically impossible to encounter accidentally (need to type or Elements()).

@aplavin
Copy link
Member Author

aplavin commented Jan 5, 2024

btw thanks a lot for all the work you put into accessors already this year 😄

Yeah, from time to time I look at AccessorsExtra and my adhoc scripts to see what can and makes sense to be upstreamed here :)

@aplavin aplavin changed the title add shortcuts for Elements() and Properties() add shortcuts for Elements(), Properties(), @optic Jan 5, 2024
@jw3126
Copy link
Member

jw3126 commented Jan 5, 2024

Especially for Elements, getindex seems quite natural visually: xxx[∗]. What exactly is the "collision" you have in mind? Note that (\ast) is not *, so it's not type piracy! It's a unicode shortcut, I think it's fine to have unicode symbols as shortcuts as long as it's not the only way.

= 3
f = @optic _[]
@assert f([1,2,42]) == 42 # will fail with this proposal

is a gotcha.

@aplavin
Copy link
Member Author

aplavin commented Jan 5, 2024

Heh, nice edge case :) We can fix that by always resolving within @optic context as Accessors.∗.
Oh, and not even export these symbols to avoid potential clashes.

@aplavin
Copy link
Member Author

aplavin commented Jan 5, 2024

After some thinking, I believe the regular export approach is both most straightforward, and least like to cause inconsistencies. Your example is similar to someone defining a variable/function/... named Elements or If: the effect is the same, corresponding Accessors exports become shadowed in this case. That's just how Julia works.

@jw3126
Copy link
Member

jw3126 commented Jan 6, 2024

Your example is similar to someone defining a variable/function/... named Elements or If: the effect is the same, corresponding Accessors exports become shadowed in this case. That's just how Julia works.

In these cases the correct scope is used:

using Accessors

struct Elements end

(::Elements)(x) = 2*x


o = @optic _ |> Elements()

@assert o(1)  == 2 # works

Comment on lines +437 to +441
# Elements, Properties
const ∗ = Elements()
const ∗ₚ = Properties()
IndexLens(::Tuple{Elements}) = Elements()
IndexLens(::Tuple{Properties}) = Properties()
Copy link
Member

Choose a reason for hiding this comment

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

This looks much better than doing it at "parse time". Can you add a test where the user defines these symbols?

Copy link
Member

Choose a reason for hiding this comment

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

Optionally we could add a layer of indirection and instead of overloading the IndexLens constructor with something that is not an IndexLens, do this dispatch in another function index_like_lens.

@jw3126
Copy link
Member

jw3126 commented Jan 6, 2024

Oh I think I misread your code from the very beginning. The problem I imagined with symbols being pulled from the wrong scope might never have existed. Can you add a testcase like this:

= 3
f = @optic _[]
@test f([1,2,42]) == 42

and then it is good to go.

@aplavin
Copy link
Member Author

aplavin commented Jan 6, 2024

And I also misread your "gotcha example", thinking that you suggest that should always be resolved as Elements() no matter what the user has defined :)

Added tests.

@aplavin aplavin merged commit d5bf87b into JuliaObjects:master Jan 6, 2024
6 of 8 checks passed
@aplavin aplavin deleted the shortcuts branch January 6, 2024 13:30
@aplavin
Copy link
Member Author

aplavin commented Jul 2, 2024

The @o shortcut is so nice, I really enjoy using it :)
Even shorter than an anonymous function (same length with brackets), but reads cleaner somehow:

x -> x.a + 1
@o _.a + 1
(@o _.a + 1)

@jw3126
Copy link
Member

jw3126 commented Jul 3, 2024

I also like it.

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.

2 participants