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

Return an empty collection instead of null #2530

Merged

Conversation

sugmanue
Copy link
Contributor

Background

The method get<MemberName>OrEmpty() for collections, either lists or maps, was supposed to return an empty collection when the collection was not set, instead of null, but somehow we missed to add this expected behavior. This change fixes this, if the collection was not set we return either Collections.emptyList() or
Collections.emptyMap() depending on the collection type instead of null. The user can still find whether the value was set by using the Optional<...> get<MemberName>() variant that returns en empty optional when the collection was not set.

Testing

Validated the generated code. We don't currently have asserts for the generated code but only tests that asserts that everything compiles as expected. Asserting on the expected generated code would be good but can be added in a different change.

For the structure trait in the tests now the code generated look like this for the members fieldD and fieldE.

    public Optional<List<String>> getFieldD() {
        return Optional.ofNullable(fieldD);
    }

    public List<String> getFieldDOrEmpty() {
        return fieldD == null ? Collections.emptyList() : fieldD;
    }

    public Optional<Map<String, String>> getFieldE() {
        return Optional.ofNullable(fieldE);
    }

    public Map<String, String> getFieldEOrEmpty() {
        return fieldE == null ? Collections.emptyMap() : fieldE;
    }

Links

  • Links to additional context, if necessary
  • Issue #, if applicable (see here for a list of keywords to use for linking issues)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The method `get<MemberName>OrEmpty()` for collections, either lists or
maps, was supposed to return an empty collection when the collection
was not set, instead of null, but somehow we missed to add this
expected behavior. This change fixes this, if the collection was not
set we return either `Collections.emptyList()` or
`Collections.emptyMap()` depending on the collection type instead of
`null`. The user can still find whether the value was set by using the
`Optional<...> get<MemberName>()` variant that returns en empty
optional when the collection was not set.
@sugmanue sugmanue requested a review from a team as a code owner February 19, 2025 03:46
@sugmanue sugmanue requested a review from kstich February 19, 2025 03:46
Comment on lines 148 to 149
Collections.class,
isListShape ? "List" : "Map"));
Copy link
Contributor

@haydenbaker haydenbaker Feb 19, 2025

Choose a reason for hiding this comment

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

Could you do just Collections.emptyList()/Collections.emptyMap() instead of having to interpolate with empty? It would be a tad clearer, but no big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can, but we need to branch somewhere. What about spelling emptyList and emptyMap instead of just List and Map. That might be clearer.

@sugmanue sugmanue merged commit 595a391 into smithy-lang:main Feb 19, 2025
7 checks passed
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