Comments inline.
On Wed, Nov 10, 2021 at 1:17 AM Milles, Eric (TR Technology) <
eric.mil...@thomsonreuters.com> wrote:
> A couple questions regarding record types:
>
>
>
> record Person(String name, Date dob) {
>
> public Person {
>
> // ...
>
> }
>
> }
>
>
>
> 1) Was it by design to have Person(String) and Person() constructors
> created for the example above? I would prefer to see the canonical
> constructor and the map constructor only. That is,
> TupleConstructor(defaults=false). Then if I wanted the others, I can add
> default arguments to any of the components or set defaults to true
> explicitly.
>
The current implementation is by design. "@RecordType" expands to (among
other things) "@TupleConstructor(namedVariant = true, force = true)" and
doesn't include "defaults = false". The way we do our merging given we
currently have PREFER_EXPLICIT_MERGED as the collector mode meant that if I
had "defaults = false" explicitly it wasn't allowing "defaults = true" to
be added. Merging was overriding it. We possibly need to re-look at that
merging behavior and then flipping the defaults boolean is a workable way
forward.
> 2) Is the modifier required for the compact constructor by design? If I
> remove "public" above it fails to parse. Java allows the compact
> constructor without visibility modifier.
>
That was not easy to do with the current grammar hence the limitation. I
was hoping we would find a way to remove the limitation before 4GA hence it
possibly isn't mentioned yet in the documentation/release notes. The
working of how the compact constructor was "plumbed in" has changed
slightly - perhaps we can re-look whether removing that limitation is
easier now.
> 3) Did the copyWith() or components() ideas get dropped? I am not seeing
> them in 4.0b2.
>
They currently default to false but you can turn them on with the
respective boolean annotation attributes in RecordOptions. I set them to
false because:
* copyWith() is also false in ImmutableBase, so I left it at the same
default value as that for consistency and you can get a long way with
toList/toMap and the existing constructors.
* components() only works for records having a limited number of components
up to our largest TupleN class (currently Tuple16), so I figured users who
turn that on will know what they are doing and won't be surprised if they
get a compiler error for 17 components (say)
4) Since the compact constructor is implemented via
> TupleConstructor(pre=...), there is no error or warning if I provide a pre
> closure and a compact constructor. Should there be an error or warning?
>
I am not sure it would always be an error/warning or if some natural
merging could be feasible but I suspect it isn't covered well by existing
tests. Any help in that area would be greatly appreciated.
>
>
>
>
> *From:* OCsite <o...@ocs.cz>
> *Sent:* Tuesday, November 2, 2021 2:07 PM
> *To:* MG <mg...@arscreat.com>
> *Cc:* Groovy_Developers <dev@groovy.apache.org>; pa...@asert.com.au
> *Subject:* [EXT] Re: Record enhancements
>
>
>
> *External Email:* Use caution with links and attachments.
>
>
>
> As for tersity, I presume the actual usage would look like „foo as Map“ or
> „foo as List“ anyway, which is actually one less keypress :), and — which
> in my personal opinion is considerably more important — it offers better
> consistency and polymorphism.
>
>
>
> (I know next to nothing of Intellisense, but I guess it should offer the
> *as* operator with a selection of known types, should it not?)
>
>
>
> Still I might be missing something of importance, of course.
>
>
>
> All the best,
>
> OC
>
>
>
> On 2 Nov 2021, at 19:17, MG <mg...@arscreat.com> wrote:
>
>
>
> Hmmm, yes, that would be an option.
> More terse & can be discovered via Intellisense are two reasons I could
> think of that speak for the toList()/toMap() approach...
>
> Cheers,
> mg
>
> On 02/11/2021 12:48, OCsite wrote:
>
> Hi there,
>
> I am probably missing something obvious here, but why adding separate
> methods for this instead of simply reusing asType?
>
> Thanks and all the best,
> OC
>
>
> On 2. 11. 2021, at 8:35, Paul King <pa...@asert.com.au> wrote:
>
> Thanks for the feedback! I added "toMap()" to the PR.
>
> On Tue, Nov 2, 2021 at 7:02 AM MG <mg...@arscreat.com> wrote:
>
> Hi Paul,
>
> quick "from the top of my head" reply:
>
> copyWith(...): Sounds like a great idea, I have record-like classes in
> use, and the need for something like this arises immediately in practice
> getAt(int): Don't see why not, might be useful
> toList(): Destructuring should show its strength when pattern matching is
> introduced - outside of pattern matching right now I don't see much
> application/need for such functionality (but would be interested to see
> some practical examples :-) ), but having it does not seem to hurt.
> components(): Same as getAt; the name seems quite long, maybe we can come
> up with something more terse ?
> toMap(): If we have toList(), would a toMap() make sense, so that the map
> could be modified and passed as a record ctor argument to create a new
> record ?
>
> Cheers,
> mg
>
>
> On 01/11/2021 16:14, Paul King wrote:
>
> Hi folks,
>
> I will be ready for a new Groovy 4 release shortly. I am interested in
> folks' thoughts on records as they have gone through a few changes
> recently (documented in [1], [2] and [3]) and there is a proposal[4]
> for a few more enhancements.
>
> There is a "copyWith" method (still undergoing some refactoring)
> similar to the copy method in Scala and Kotlin which allows one record
> to be defined in terms of another. It can be disabled if you really
> must have Java-like records. The refactoring of that method hit a
> slight glitch, so might not work if you grab the latest source but
> should be fixed shortly.
>
> record Fruit(String name, double price) {}
> def apple = new Fruit('Apple', 11.6)
> assert apply.toString() == 'Fruit[name=Apple, price=11.6]'
> def orange = apple.copyWith(name: 'Orange')
> assert orange.toString() == 'Fruit[name=Orange, price=11.6]'
>
> There is a "getAt(int)" method to return e.g. the first component with
> myRecord[0] following similar Groovy conventions for other aggregates.
> This is mostly targeted at dynamic Groovy as it conveys no typing
> information. Similarly, there is a "toList" (current name but
> suggestions welcome) method which returns a Tuple (which is also a
> list) to return all of the components (again with typing information).
>
> record Point(int x, int y, String color) {}
> def p = new Point(100, 200, 'green')
> assert p[0] == 100
> assert p[1] == 200
> assert p[2] == 'green'
> def (x, y, c) = p.toList()
> assert x == 100
> assert y == 200
> assert c == 'green'
>
> There is also an optional (turned on by an annotation attribute)
> "components" method which returns all components as a typed tuple,
> e.g. Tuple1, Tuple2, etc. This is useful for Groovy's static nature
> and is automatically handled by current destructuring (see the tests
> in the PR). The limitation is that we currently only go to Tuple16
> with our tuple types - which is why I made it disabled by default.
>
> @RecordBase(componentTuple=true)
> record Point(int x, int y, String color) { }
>
> @TypeChecked
> def method() {
> def p1 = new Point(100, 200, 'green')
> def (int x1, int y1, String c1) = p1.components()
> assert x1 == 100
> assert y1 == 200
> assert c1 == 'green'
>
> def p2 = new Point(10, 20, 'blue')
> def (x2, y2, c2) = p2.components()
> assert x2 * 10 == 100
> assert y2 ** 2 == 400
> assert c2.toUpperCase() == 'BLUE'
> }
>
> An alternative would be to follow Kotlin's approach and just have
> typed methods like "component1", "component2", etc. We might want to
> follow that convention or we might want to follow our TupleN naming,
> e.g. "getV1", "getV2", etc. We would need to augment the Groovy
> runtime and type checker to know about records if we wanted to support
> destructuring but we could avoid the "toList" method and "components"
> method with its size limitation if we did add such support.
>
> Any feedback welcome,
>
> Cheers, Paul.
> P.S. Records are an incubating feature - hence may change in backwards
> incompatible ways, particularly until we hit Groovy 4 final.
>
> [1]
> https://github.com/apache/groovy/blob/master/src/spec/doc/_records.adoc
> <https://urldefense.com/v3/__https:/github.com/apache/groovy/blob/master/src/spec/doc/_records.adoc__;!!GFN0sa3rsbfR8OLyAw!NyOeTO41BsAx72RYKaFkHpjzc7rfbQO2ajvOqriwA1vxqYxsGsMsTEVmIQKC-qtDO--XcfI$>
> [2]
> https://github.com/apache/groovy/blob/master/src/spec/test/RecordSpecificationTest.groovy
> <https://urldefense.com/v3/__https:/github.com/apache/groovy/blob/master/src/spec/test/RecordSpecificationTest.groovy__;!!GFN0sa3rsbfR8OLyAw!NyOeTO41BsAx72RYKaFkHpjzc7rfbQO2ajvOqriwA1vxqYxsGsMsTEVmIQKC-qtDCIm0GQs$>
> [3]
> https://github.com/apache/groovy-website/blob/asf-site/site/src/site/wiki/GEP-14.adoc
> <https://urldefense.com/v3/__https:/github.com/apache/groovy-website/blob/asf-site/site/src/site/wiki/GEP-14.adoc__;!!GFN0sa3rsbfR8OLyAw!NyOeTO41BsAx72RYKaFkHpjzc7rfbQO2ajvOqriwA1vxqYxsGsMsTEVmIQKC-qtDPjyENMk$>
> [4] https://issues.apache.org/jira/browse/GROOVY-10338
> <https://urldefense.com/v3/__https:/issues.apache.org/jira/browse/GROOVY-10338__;!!GFN0sa3rsbfR8OLyAw!NyOeTO41BsAx72RYKaFkHpjzc7rfbQO2ajvOqriwA1vxqYxsGsMsTEVmIQKC-qtDt01uPzI$>
>
>
>
>
>
