Skip to content

[RFC] monkey patch setindex #107

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

Merged
merged 6 commits into from
Dec 10, 2019
Merged

[RFC] monkey patch setindex #107

merged 6 commits into from
Dec 10, 2019

Conversation

jw3126
Copy link
Owner

@jw3126 jw3126 commented Dec 9, 2019

It is a recurring annoyance for me that I want to @set arr::Array and cannot do it.

Should we allow doing that?
Should we use our own variant of Base.setindex?
Since BangBang also needs this and implements it much better then this PR, should that function be part of ConstructionBase? Or should it be in a NoBang package?

What do you think @tkf?

Copy link
Collaborator

@tkf tkf left a comment

Choose a reason for hiding this comment

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

This PR LGTM! I did want to use this occasionally, too (e.g., https://github.com/queryverse/VegaLite.jl/pull/191/files#r333284529).

Since BangBang also needs this and implements it much better then this PR, should that function be part of ConstructionBase? Or should it be in a NoBang package?

I think it'd be a slight stretch to put this in ConstructionBase. We can also extract NoBang as a separate "base" package and put it in JuliaObjects organization (with a better name; maybe something like ImmutableInterfaces). But, as a short term solution, I think it's good to have a copy of definition in Setfield, especially until when there is a need for overloading setindex.

(For a long term solution, I hope this becomes a part of Julia Base. FYI, I sent a PR/RFC in Julia Base JuliaLang/julia#33495 to add similar definitions. But it does not get any attention yet.)

BTW, a super nitpicky comment: I don't think this PR includes any monkey patching or type piracy. It just cleanly extends Base.setindex without modifying (patching) global state by introducing internal Setfield.setindex.

@tkf
Copy link
Collaborator

tkf commented Dec 9, 2019

I made #108 to add BangBang-like type-widening semantics to Setfield.setindex. I think it'd be more consistent with how other lenses work (e.g., set((1, 2), (@lens _[1]), 1.23)). What do you think?

@jw3126
Copy link
Owner Author

jw3126 commented Dec 9, 2019

I think it would be a good idea to move this to a separate package in the JuliaObjects organization. I am fine with ImmutableInterfaces.jl.

@tkf
Copy link
Collaborator

tkf commented Dec 10, 2019

After implementing #108, I realized that it's hard to do this outside of Base. We either need to:

  1. Cover whole Julia ecosystem via @require as I did for StaticArrays in Allow changing type via Setfield.setindex #108, or
  2. Convince other packages to define both ImmutableInterfaces.setindex and Base.setindex.

But since option 1 is the only direction that is doable on our side, I guess we don't have much choice but to use option 1...

ATM, there is a small "circular" dependency from BangBang to NoBang for implementing append(xs::AbstractVector, ys::AbstractVector): https://github.com/tkf/BangBang.jl/blob/6296b2a87d7051830a1a836b13ba3dd4d74ee052/src/NoBang/base.jl#L48. I need to think about doing this efficiently for completely decoupling NoBang from BangBang.

Meanwhile, how about just merging this PR as-is? I guess there are not many demerits for doing this?

@jw3126
Copy link
Owner Author

jw3126 commented Dec 10, 2019

I agree. Can you maybe rebase JuliaLang/julia#33495 and we try to convince people to merge it?

@jw3126 jw3126 merged commit c881b6c into master Dec 10, 2019
@tkf
Copy link
Collaborator

tkf commented Dec 10, 2019

I resolved the conflict and also ping people in #arrays channel of slack. (But this never worked...)

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