-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 NamedTuple#update(other : NamedTuple) to merge values but keep type #4727
Add NamedTuple#update(other : NamedTuple) to merge values but keep type #4727
Conversation
Looks like better to name: updated_options = default_options.with(options)
# or
updated_options = default_options.apply(options) |
In my opinion, there should be one method of this kind, the most universally useful one (#4688). The fringe benefits don't justify the complexity. |
NamedTuple is a mean to easily pass hash-like data around, for methods that may or may not consume passed values. The If you want more strictness, you should define an explicit configuration struct. |
Custom structs cannot easily be passed as method arguments. I think this is an important use-case for configuration/options as a NamedTuple. When you use a non-strict |
Can we close this since #4688 was merged? |
Why was this closed? This PR was intended to be merged alongside #4688 and #2634. They're not per se mutually exclusive but could live side by side with different semantics as in #4688 (issue comment). I don't want to put to much pressure on this, but I don't think it's a reasonable explanation to close this and #2634 just because #4688 was merged. They propose to do something entirely different and imo provide a substantial benefit for working with named parameter options and default values. A real example is merge_options in sass.cr: |
@straight-shoota I don't follow why the NamedTuple.merge is not enough to handle options with default values. A tuple with additional entries can/should be used always where a tuple with less entries is required. They can just not be used and be ignored and the program should make sense. The only place where the compiler can't use a tuple with additional entries is when splat is introduced and the method has strict named argument. In this case the compiler will complain on the splat call and should be enough guidance to fix the issue. Why does extra options complicate things in your scenario? |
@bcardiff I think that main issue here is keeping the same type for a given def foo?(**options)
default_options = {
unicorn: true,
}
options = default_options.merge(options)
bar!(**options)
end
def bar!(unicorn : Bool)
if unicorn
puts "Yay!"
else
raise "Oh no!"
end
end
foo?(unicorn: 0xdeadbeef) Since
|
@bcardiff I'm not sure if |
I'm actually not sure that adding this method simply to error a bit earlier in type checking is a very good idea. I think that using named tuples as "options objects" should be quite rare with named arguments, and i'm not too sure of a usecase outside that. I don't think we should add this method if this is the only usecase. |
Okay, I think we can close this. There seems to be not much benefit from this method. |
@straight-shoota one last though about this. I think that early type checking the options is a bit too much. But if one could do that something like alias Config = NamedTuple(host: String, port: Int32)
opt1 = { host: "localhost", port: 80 }
Config.new(**opt1)
opt2 = { host: "localhost", port: "80" }
Config.new(**opt2) # compile error Adding extra options will also generate a compile time error. Which I don't like for this use case since it should not bother. To solve that what we could be missing is a slice method for tuples/namedtuple that would allow to project a reduced tuple with type information. But those helpers are doable upon need. |
This method is similar to #4688 and #2634 with different semantics as proposed in #4688 (comment):
NamedTuple#update(other : NamedTuple)
creates a new instance of self's NamedTupe type with values ofself
updated with those ofother
.other
's keys need to be a subset ofself
's keys. Inversion is only valid if the key sets are identical. The behavior is similar to#merge
but it ensures that no additional keys are added and preserves the types fromself
.A typical use case would be merging a fixed set of options with default values while disallowing additional keys (in contrast to
#merge
)I am not super happy with the name, because it does not actually update
self
but returns a new NamedTuple. So if there are any suggestions...