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

Merged
merged 4 commits into from Jun 12, 2018

Conversation

Projects
None yet
10 participants
@chris-baynes
Contributor

chris-baynes commented Feb 22, 2018

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:

record Point, x : Int32, y : Int32, z : Int32
p1 = Point.new(3, 4, 5)
p1.copy y: 9 # => #<Point(@x=3, @y=9, @z=5)>

The inspiration for this comes from Scala case class copy: https://docs.scala-lang.org/tour/case-classes.html

@RX14

This comment has been minimized.

Member

RX14 commented Feb 22, 2018

My first intuition would be to call this copy_with, as I think it reads better. I really like the idea though, I used groovy's copyWith a fair bit.

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 make crystal followed by bin/crystal format to format your PR)

@jhass

This comment has been minimized.

Member

jhass commented Feb 22, 2018

I could live with copy_with, but how about update?

@z64

This comment has been minimized.

z64 commented Feb 22, 2018

IMO, update carries a notion of mutability, or is ambiguous. copy is a bit more explicit; regardless of whether the receiver is mutable or not, you understand a copy is being made.

@jhass

This comment has been minimized.

Member

jhass commented Feb 22, 2018

The primary contract for this method is not copying though, it's creating a new instance with some values changed. For copying we have dup and clone already. I would tend to argue that the copy is "just" a (necessary) side effect of changing values on an immutable object.

update's heritage would be Hash#update.

@MakeNowJust

This comment has been minimized.

Contributor

MakeNowJust commented Feb 22, 2018

Scala calls such a method as clone. I misunderstood, copy is correct.

@RX14

This comment has been minimized.

Member

RX14 commented Feb 22, 2018

I'm happy with update. clone implies there's nothing actually modified about the clone, same with copy.

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Feb 22, 2018

We don't even need dup, because a record is a struct; just b = a; b.y = 9 copies the record and updates a field.

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 ?

@Sija

This comment has been minimized.

Contributor

Sija commented Feb 22, 2018

@ysbaddaden with the difference that record defines attributes as getters, thus making it immutable.

@RX14

This comment has been minimized.

Member

RX14 commented Feb 22, 2018

Yes, this is exactly the point. Records are immutable objects and this is a nice and clean abstraction for "mutating" them.

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Feb 23, 2018

Records are just structs (sugar definition), they can have methods that mutate instance variables; they're not immutable.

@RX14

This comment has been minimized.

Member

RX14 commented Feb 23, 2018

@ysbaddaden but by default they are immutable. People typically use them as immutable, or they would just define a struct with property macros, as it's shorter than a record when it's mutable.

Please consider the typical usecase.

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Feb 23, 2018

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 copy method, even thought the concept doesn't stand anymore (or never did).

BTW: #update is confusing (feels like it's mutating the object), better keep #copy or specialize #dup to accept arguments, which would fit better and avoid adding a new concept.

@RX14

This comment has been minimized.

Member

RX14 commented Feb 23, 2018

@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 def_copy_with method which would work on any class or struct (as long as it has the right ctor which accepts named arguments for all ivars).

@chris-baynes

This comment has been minimized.

Contributor

chris-baynes commented Feb 23, 2018

Have formatted the code. About the naming, I couldn't find many other functional languages that have a named method for this:
groovy: copyWith
kotlin, scala: copy
elm, haskell, erlang, elixir: don't have a function, it's done with syntactic sugar though the process is generally referred to as "update"
My personal favourite is still copy.

@yxhuvud

This comment has been minimized.

Contributor

yxhuvud commented Mar 5, 2018

Personally I'd favor to instead create a dup that takes an argument, ie record.dup(y: 4). Easier to remember, and doesn't really create any ambigousness that I can see.

@chris-baynes

This comment has been minimized.

Contributor

chris-baynes commented Mar 27, 2018

So is that a consensus that this should be called dup then? Would be happy to make the change to get this merged. I still think this is a useful feature and have wanted to use it in code.

@RX14

This comment has been minimized.

Member

RX14 commented Mar 27, 2018

I don't think it should be called dup. Maybe dup_with... Even then I prefer copy_with. I don't care too much though, as long as it's in I'll bear the naming.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Mar 27, 2018

Why not just call it with? It's short and precise. I don't think it necessarily needs a verb.

point = Point.new(3, 4, 5)
point.with(y: 9)

copy_with would be okay as well, but longer.

@chris-baynes chris-baynes force-pushed the contiamo:add_record_copy branch from eafd209 to 44a244c Jun 4, 2018

chris-baynes added some commits Feb 21, 2018

Add copy method to record macro.
This allows a copy of a record to be made, specifying
only the properties that should be changed.

@chris-baynes chris-baynes force-pushed the contiamo:add_record_copy branch from 44a244c to d9f59c1 Jun 5, 2018

@chris-baynes

This comment has been minimized.

Contributor

chris-baynes commented Jun 6, 2018

I've renamed it to copy_with and rebased. Anything else I can do to get this merged?

@jhass

jhass approved these changes Jun 6, 2018

@@ -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:

This comment has been minimized.

@RX14

RX14 Jun 6, 2018

Member

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.

@RX14

This comment has been minimized.

Member

RX14 commented Jun 6, 2018

Apart from the docs, it's a big +1 from me

@chris-baynes

This comment has been minimized.

Contributor

chris-baynes commented Jun 7, 2018

Ok, I've updated the docs

@bcardiff

One minor comment and a request for another name for this.

#
# p = Point.new y: 2 # => #<Point(@x=0, @y=2)>
# p.copy_with x: 3 # => #<Point(@x=3, @y=2)>
# ```

This comment has been minimized.

@bcardiff

bcardiff Jun 7, 2018

Member

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)>

This comment has been minimized.

@chris-baynes

chris-baynes Jun 11, 2018

Contributor

Fixed

@@ -67,6 +77,30 @@ macro record(name, *properties)

{{yield}}

def copy_with({{

This comment has been minimized.

@bcardiff

bcardiff Jun 7, 2018

Member

I really think the method should be called merge since it mimic the NamedTuple#merge method.

This comment has been minimized.

@RX14

RX14 Jun 7, 2018

Member

@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.

@chris-baynes chris-baynes force-pushed the contiamo:add_record_copy branch from e2d4210 to 61c23a0 Jun 8, 2018

@RX14

RX14 approved these changes Jun 11, 2018

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jun 12, 2018

I still think merge should be the name of the method. If we don't then named tuples and records will have different api.

Changing a variable from NamedTuple(x: Int32, y: Int32) to a record(x: Int32, y: Int32) will requiere extra changes.

Yes, they do have a different API since reader are different tuple[:foo] vs record.foo. That difference is to capture which properties exists or not. Maybe it could go away, or not. But making merge and copy_with different is an unnecessary IMO. And is also consistent with Hash#merge.

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.

@RX14

This comment has been minimized.

Member

RX14 commented Jun 12, 2018

I completely disagree. Named tuples and records's APIs are different because they're two different data structures: one is an immutable Hash, and the other is an immutable OO struct with it's own class and can be inherited and have methods. Working to make the APIs the same makes no sense: you can restrict for NamedTuple and be sure whatever you're operating on has an update because it's a named tuple, but no such type restriction exists for "all records". You'll never be operating on "all records" the same way you can write a method that operates on all named tuples. That's because a named tuple is a collection, and a record is not, it's just a plain class.

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.

@RX14

This comment has been minimized.

Member

RX14 commented Jun 12, 2018

And thinking about it more, this PR is the first instance of record adding any API which is specific to records. Records don't have an API because they're just a shortcut for defining a struct.

So we should add a includeable module that adds copy_with for any class or struct, and then use that in record, to avoid giving records any special API.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jun 12, 2018

@RX14 I generally agree.

Records don't have an API

I'd consider the constructor receiving named arguments a API method specific to records. And copy_with is just a variant of that: It creates a new instance with self's properties as defaults.

We could certainly discuss a generalized copy_with, but I'm not sure how that's supposed to work as a re-usable module method, because the implementation is very specific to record and only works with it's special constructor.

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.

@bcardiff

I still don't like it.

@RX14 RX14 added this to the 0.25.1 milestone Jun 12, 2018

@RX14 RX14 merged commit 0a898dd into crystal-lang:master Jun 12, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment