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

Issue #123: support nullable collections #156

Merged

Conversation

hofiisek
Copy link
Contributor

Solves #123

There's a new allowNullableCollections option that does the following:

  • if set to false (default value for BC), does nothing
  • if set to true and interpretNotNulls = false, a list/set/map/collection record component will return null if value is not set
  • if set to true and interpretNotNulls = true, a list/set/map/collection record component will return null only if it's determined to be nullable, i.e. isn't annotated by any of the not-null patterns

All branches mentioned above should be covered by the TestNullableCollectionsBuilder test

@Randgalt
Copy link
Owner

Thanks for this. I'll have a look very soon


import static io.soabase.recordbuilder.processor.RecordBuilderProcessor.generatedRecordBuilderAnnotation;
import static io.soabase.recordbuilder.processor.RecordBuilderProcessor.recordBuilderGeneratedAnnotation;

class CollectionBuilderUtils {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@Randgalt
Copy link
Owner

Randgalt commented Nov 19, 2023

Thanks for this PR - it looks very good.

I tried a class that only has @RecordBuilder.Options(allowNullableCollections = true) and it's a NOP regarding specialized collections. Should we emit a warning or error? Or, should allowNullableCollections = true always imply enabling useImmutableCollections?

@hofiisek
Copy link
Contributor Author

hofiisek commented Nov 20, 2023

Thanks for this PR - it looks very good.

I tried a class that only has @RecordBuilder.Options(allowNullableCollections = true) and it's a NOP regarding specialized collections. Should we emit a warning or error? Or, should allowNullableCollections = true always imply enabling useImmutableCollections?

I see, didn't think of that combination before.

Since NOP is the expected behavior in this case (components and builder getters just return whatever you set to them, without any shim calls etc.), I think we can emit a warning that it will have no effect. I'd rather not enableuseImmutableCollections automatically as it changes behavior and it could possibly break someone's project (even though I would think that the majority uses unmodifiable/immutable collections anyway).

@Randgalt
Copy link
Owner

Since NOP is the expected behavior in this case (components and builder getters just return whatever you set to them, without any shim calls etc.), I think we can emit a warning that it will have no effect. I'd rather not enableuseImmutableCollections automatically as it changes behavior and it could possibly break someone's project (even though I would think that the majority uses unmodifiable/immutable collections anyway).

A warning (or error) would be good. I'd prefer that to doing nothing.

@hofiisek
Copy link
Contributor Author

Agree. Added in the last commit :)

@Randgalt Randgalt merged commit e7b7963 into Randgalt:master Nov 23, 2023
2 checks passed
@Randgalt
Copy link
Owner

Thank you for this

@Randgalt Randgalt added this to the v38 milestone Nov 23, 2023
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