Skip to content
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

Collection copying only when changed #116

Conversation

freelon
Copy link
Contributor

@freelon freelon commented May 22, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

freelon commented May 23, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

freelon commented May 23, 2022

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

@freelon
Copy link
Contributor Author

freelon commented Jun 6, 2022

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

@Randgalt
Copy link
Owner

Randgalt commented Jun 6, 2022

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

@Randgalt Randgalt added this to the v34 milestone Jun 12, 2022
@Randgalt Randgalt merged commit 479f7f3 into Randgalt:master Jun 12, 2022
@Randgalt
Copy link
Owner

LGTM - thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants