-
-
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 copy method to record macro. #5736
Conversation
My first intuition would be to call this Please allow the rest of the team to weigh in on the design before making any changes though. Right now, the formatter is failing (please use |
I could live with |
IMO, |
The primary contract for this method is not copying though, it's creating a new instance with some values changed. For copying we have
|
|
I'm happy with |
We don't even need dup, because a Honestly, I don't see the benefit of a copy with some new values in a mutable, non functional language. Furthermore, why should the feature be restricted to record? What about structs and classes ? |
@ysbaddaden with the difference that |
Yes, this is exactly the point. |
Records are just structs (sugar definition), they can have methods that mutate instance variables; they're not immutable. |
@ysbaddaden but by default they are immutable. People typically use them as immutable, or they would just define a Please consider the typical usecase. |
What I'm saying is that nothing is immutable in Crystal, and that records are just sugar for defining pseudo-immutable structs. Crystal doesn't know about them being records, hence doesn't treat records differently than structs, and documents them as structs, because this is what they really are. Shall we specialize records under these conditions? If we add that one method only, I guess it's okayish and fits into the concept, but it's one step that makes it harder to change a record into a struct —because someone may rely on the BTW: |
@ysbaddaden sure it doesn't enforce immutability per-se but it means you have to go out of your way to mutate the struct in-place. Regarding the substitutability of a record and a struct with getters, that's a really good point. Perhaps it would be possible to add a |
Have formatted the code. About the naming, I couldn't find many other functional languages that have a named method for this: |
Personally I'd favor to instead create a dup that takes an argument, ie |
So is that a consensus that this should be called |
I don't think it should be called |
Why not just call it point = Point.new(3, 4, 5)
point.with(y: 9)
|
This allows a copy of a record to be made, specifying only the properties that should be changed.
I've renamed it to |
src/macros.cr
Outdated
@@ -46,6 +46,15 @@ | |||
# Point.new # => #<Point(@x=0, @y=0)> | |||
# Point.new y: 2 # => #<Point(@x=0, @y=2)> | |||
# ``` | |||
# | |||
# An example of creating a copy of a record with some properties changed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs should be phrased in a more informational style, for example:
The record macro also adds a
copy_with
method, which returns a copy of the record with the provided properties altered to the provided values.
Apart from the docs, it's a big +1 from me |
Ok, I've updated the docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment and a request for another name for this.
src/macros.cr
Outdated
# | ||
# p = Point.new y: 2 # => #<Point(@x=0, @y=2)> | ||
# p.copy_with x: 3 # => #<Point(@x=3, @y=2)> | ||
# ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample could show that p is not changed
point = Point.new y: 2 # => #<Point(@x=0, @y=2)>
point.copy_with x: 3 # => #<Point(@x=3, @y=2)>
point # => #<Point(@x=0, @y=2)>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -67,6 +77,30 @@ macro record(name, *properties) | |||
|
|||
{{yield}} | |||
|
|||
def copy_with({{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think the method should be called merge
since it mimic the NamedTuple#merge
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcardiff no, it shouldn't. If you have two named tuples, you can merge them. If you have a record and a named tuple, they're different types so you can't merge them. This methods isn't really meant to be used with named tuples either, it's meant to be used with named arguments - and the fact that it's using a named tuple is an implementation detail. copy_with
reads much better.
I still think Changing a variable from Yes, they do have a different API since reader are different The fact that a merge in tuple can return a tuple with more components and that the merge in records prevents extra properties does not change that the operation is merging self with subset. |
I completely disagree. Named tuples and records's APIs are different because they're two different data structures: one is an immutable These classes and usecases are so different that making the APIs the same doesn't make sense. If you want to do that, lets have that discussion in another PR. |
And thinking about it more, this PR is the first instance of record adding any API which is specific to So we should add a includeable module that adds |
@RX14 I generally agree.
I'd consider the constructor receiving named arguments a API method specific to records. And We could certainly discuss a generalized But, I'd rather just merge this PR as is, adding `copy_with´ to the record macro. Then we can talk how this could be fit in a re-usable module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't like it.
This allows a copy of a record to be made, specifying properties that should be changed.
It's useful to be able to do this when there are more than a couple of properties and you want a new record identical to the old one but with one or two properties changed. It maintains immutability while improving usability.
Example:
The inspiration for this comes from Scala case class copy: https://docs.scala-lang.org/tour/case-classes.html