Randgalt / record-builder

Record builder generator for Java records
Apache License 2.0
756 stars 55 forks source link

Collection copying only when changed #116

Closed freelon closed 2 years ago

freelon commented 2 years ago

For #114

Currently the test TestCollections.testRecordBuilderOptionsCopied fails, since I had to change the signature of the __list() shim methods. The problem is, that the code as on master generates setters for collections with different signatures: If addSingleItemCollectionBuilders == true: someList(Collection<? extends ListItem> someList), but otherwise the someList parameter is of type List<...>. I didn't want to change that behavior together with the other ticket, though (see #117 ).

Randgalt commented 2 years ago

Currently the test TestCollections.testRecordBuilderOptionsCopied fails, since I had to change the signature of the __list() shim methods. The problem is, that the code as on master generates setters for collections with different signatures:

Instead of making a backward incompatible change can't we add a 2 __list shims? One for List and one for Collection?

freelon commented 2 years ago

Instead of making a backward incompatible change can't we add a 2 __list shims? One for List and one for Collection?

Of course, but why keep __list(List<X> o)? It's more restrictive and does the same as __list(Collection ...). Also, it's a private method, so I'd think backwards compatibility is not much of an issue.

Randgalt commented 2 years ago

Currently the test TestCollections.testRecordBuilderOptionsCopied fails, since I had to change the signature of the __list() shim methods. The problem is, that the code as on master generates setters for collections with different signatures:

Instead of making a backward incompatible change can't we add a 2 __list shims? One for List and one for Collection?

True - but in that case the test can be fixed here right?

freelon commented 2 years ago

Yes, I already did that. I just didn't want to lead with changing tests ;)

freelon commented 2 years ago

Is there anything more to do on this or can it be merged? From my POV all the discussed points are solved.

Randgalt commented 2 years ago

I apologize - I got bogged down with work. I'll get to this soon (hopefully this week)

Randgalt commented 2 years ago

LGTM - thank you very much