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

[request] Make way fewer things Option<T> types #536

Closed
1 of 2 tasks
rickwebiii opened this issue May 17, 2022 · 5 comments
Closed
1 of 2 tasks

[request] Make way fewer things Option<T> types #536

rickwebiii opened this issue May 17, 2022 · 5 comments
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue

Comments

@rickwebiii
Copy link

rickwebiii commented May 17, 2022

Describe the feature

In the model types for each API (e.g. ec2), nearly every field is Option<T>. This means that developers have to make a bunch of boilerplate error handling by calling unwrap_or_default, ok_or, etc.

I expect that many fields are actually guaranteed to exist when making calls. For example, when calling ec2_client.describe_instances, every Instance should always have an instance_id. Otherwise, how would ever do anything with it?

This feature request is to comb through all the APIs and only use Option<T> on values that aren't guaranteed to exist.

Use Case

This significantly reduces the amount of boilerplate error handling needed around errors that should never occur.

Proposed Solution

Only mark truly optional fields and types as Option<T> and update all the builder() calls to accept required fields as parameters.

Other Information

This feature will definitely be a breaking change.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

A note for the community

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue, please leave a comment
@rickwebiii rickwebiii added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 17, 2022
@jdisanti
Copy link
Contributor

Thanks for filing!

This is definitely something we want to improve, and we're hoping to make many of these optional fields required prior to GA.

See also the discussion in #163

@jdisanti jdisanti removed the needs-triage This issue or PR still needs to be triaged. label May 18, 2022
@rickwebiii
Copy link
Author

Awesome, good to hear!

@jdisanti
Copy link
Contributor

Smithy IDL 2.0 now makes it possible to reduce the number of optional types, and our code generator has been updated to support it. I think we just need to modify the code generator to query Smithy's NullableIndex to determine which members can have the Options removed. We're getting closer! 😄

@jdisanti
Copy link
Contributor

jdisanti commented Oct 5, 2023

This will go out with the next big release (it will be announced in the release notes when it is, so if there are other releases before it, it is not in those).

@jdisanti jdisanti added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Oct 5, 2023
@rcoh rcoh closed this as completed Nov 2, 2023
Copy link

github-actions bot commented Nov 2, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@rcoh rcoh removed the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue
Projects
Archived in project
Development

No branches or pull requests

4 participants