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

Should the setters be kept in V2 #81

Closed
xibz opened this issue Jan 4, 2018 · 6 comments
Closed

Should the setters be kept in V2 #81

xibz opened this issue Jan 4, 2018 · 6 comments
Labels
guidance Question that needs advice or information.

Comments

@xibz
Copy link
Contributor

xibz commented Jan 4, 2018

With V2, we are curious if people are using the setters in the service APIs. As this isn't too idiomatic to Go, we want to ensure that there is a benefit that people are getting other than just convinience. Should we keep setters in V2?

@xibz xibz added the guidance Question that needs advice or information. label Jan 4, 2018
@harrisonhjones
Copy link

Are we talking about these kind of setters?

func (s *AddTagsToStreamInput) SetStreamName(v string) *AddTagsToStreamInput

@xibz
Copy link
Contributor Author

xibz commented Jan 4, 2018

@harrisonhjones - Yes, those are the setters in question

@itsjamie
Copy link

itsjamie commented Jan 8, 2018

Removing the setters to make the API surface smaller if they are just doing an assignment to a public field is my vote.

A smaller API surface will make the documentation nicer to read and much of the cruft will be gone allowing you to focus on the important bits.

Thanks for asking!

@jasdel
Copy link
Contributor

jasdel commented Jan 8, 2018

Thanks for the feedback. Correct these methods in question are only connivence methods to set API parameters without the need to use the aws.String or similar utilities. The additional noise these methods add to the docs is quite significant. Given the size of the APIs for most services these additional methods can make it difficult to read the docs.

This is why we're interested in your feedback. If these utility methods don't provide any unique value or functionality, it may be a good idea to remove them from the v2 SDK.

@shawnsmithdev
Copy link

shawnsmithdev commented Jan 11, 2018

This seems to come down to two facts about Go:

Thus aws.String() and related util functions exist. Which are just:

func String(value string) *string {
    return &value;
}

The idea behind these setters seems to be to let you avoid aws.String() and use your string literals or constants directly. But we pay quite a price in API surface area, and it encourages a code style that is not very idomatic Go (procedural mutation).

// idiomatic
desc, err := client.DescribeStreamRequest(&kinesis.DescribeStreamInput{
        StreamName: aws.String(stream) // or &stream if stream is not a const or literal
}).Send()

// not idiomatic
var descStreamInput kinesis.DescribeStreamInput
descStreamInput.SetStreamName(stream)
desc, err := client.DescribeStreamRequest(&descStreamInput).Send()

The style thing comes down to personal preference, but IMHO even if you prefer the setter style, the resulting API ugliness is hard to justify.

@xibz
Copy link
Contributor Author

xibz commented Jan 17, 2018

We have decided to remove the setters based off the feedback here, see #101. Thank you all for providing us with your feedback in making the SDK better :).

@xibz xibz closed this as completed Jan 17, 2018
jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this issue Mar 8, 2018
Services
---
* Synced the V2 SDK with latests AWS service API definitions.

Breaking Changes
---
* `private/mode/api`: Refactor service paginator helpers to iterator pattern ([aws#119](aws#119))
	* Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks.
	* See the linked PR for an example.
* `private/model/api`: Removes setter helpers from service API types ([aws#101](aws#101))
	* Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature.
	* Based on feedback [aws#81][aws#81]
* `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled ([aws#131](aws#131))
	* Renames CanceledErrorCode to correct naming scheme.

SDK Bugs
---
* `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string ([aws#102](aws#102))
	* Fixes SDK waiters not detecting the correct condition is met. [aws#92](aws#92)
* `aws/external`: Fix EnvConfig misspelled container endpoint path getter ([aws#106](aws#106))
	* This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface.
	* Fixes [aws#105](aws#105)
* `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen ([aws#107](aws#107))
	* Fixes the S3Crypto's handling of TagLen to only be set if present.
	* V2 Fix for [aws/aws-sdk-go#1742](aws/aws-sdk-go#1742)
* `private/model/api`: Update SDK service client initialization documentation ([aws#141](aws#141))
	* Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1.
	* Related to [aws#136](aws#136)

SDK Enhancements
---
* `aws`: Improve pagination unit tests ([aws#97](aws#97))
	* V2 port of [aws/aws-sdk-go#1733](aws/aws-sdk-go#1733)
* `aws/external`: Add example for shared config and static credential helper ([aws#109](aws#109))
	* Adds examples for the  config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc.
* `private/model/api`: Add validation to prevent collision of api defintions ([aws#112](aws#112))
	* V2 port of [aws/aws-sdk-go#1758](aws/aws-sdk-go#1758)
* `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var ([aws#128](aws#128))
	* When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail.
	* V2 port of [aws/aws-sdk-go#1799](aws/aws-sdk-go#1799)
* Add code of conduct ([aws#138](aws#138))
* Update SDK README dep usage ([aws#140](aws#140))
jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this issue Mar 8, 2018
Services
---
* Synced the V2 SDK with latests AWS service API definitions.

Breaking Changes
---
* `private/mode/api`: Refactor service paginator helpers to iterator pattern (aws#119)
	* Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks.
	* See the linked PR for an example.
* `private/model/api`: Removes setter helpers from service API types (aws#101)
	* Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature.
	* Based on feedback aws#81
* `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled (aws#131)
	* Renames CanceledErrorCode to correct naming scheme.

SDK Bugs
---
* `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string (aws#102)
	* Fixes SDK waiters not detecting the correct condition is met. aws#92
* `aws/external`: Fix EnvConfig misspelled container endpoint path getter (aws#106)
	* This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface.
	* Fixes aws#105
* `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen (aws#107)
	* Fixes the S3Crypto's handling of TagLen to only be set if present.
	* V2 Fix for aws/aws-sdk-go#1742
* `private/model/api`: Update SDK service client initialization documentation (aws#141)
	* Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1.
	* Related to aws#136

SDK Enhancements
---
* `aws`: Improve pagination unit tests (aws#97)
	* V2 port of aws/aws-sdk-go#1733
* `aws/external`: Add example for shared config and static credential helper (aws#109)
	* Adds examples for the  config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc.
* `private/model/api`: Add validation to prevent collision of api defintions (aws#112)
	* V2 port of aws/aws-sdk-go#1758
* `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var (aws#128)
	* When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail.
	* V2 port of aws/aws-sdk-go#1799
* Add code of conduct (aws#138)
* Update SDK README dep usage (aws#140)
jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this issue Mar 8, 2018
Services
---
* Synced the V2 SDK with latests AWS service API definitions.

Breaking Changes
---
* `private/mode/api`: Refactor service paginator helpers to iterator pattern (aws#119)
	* Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks.
	* See the linked PR for an example.
* `private/model/api`: Removes setter helpers from service API types (aws#101)
	* Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature.
	* Based on feedback aws#81
* `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled (aws#131)
	* Renames CanceledErrorCode to correct naming scheme.

SDK Bugs
---
* `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string (aws#102)
	* Fixes SDK waiters not detecting the correct condition is met. aws#92
* `aws/external`: Fix EnvConfig misspelled container endpoint path getter (aws#106)
	* This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface.
	* Fixes aws#105
* `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen (aws#107)
	* Fixes the S3Crypto's handling of TagLen to only be set if present.
	* V2 Fix for aws/aws-sdk-go#1742
* `private/model/api`: Update SDK service client initialization documentation (aws#141)
	* Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1.
	* Related to aws#136

SDK Enhancements
---
* `aws`: Improve pagination unit tests (aws#97)
	* V2 port of aws/aws-sdk-go#1733
* `aws/external`: Add example for shared config and static credential helper (aws#109)
	* Adds examples for the  config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc.
* `private/model/api`: Add validation to prevent collision of api defintions (aws#112)
	* V2 port of aws/aws-sdk-go#1758
* `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var (aws#128)
	* When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail.
	* V2 port of aws/aws-sdk-go#1799
* Add code of conduct (aws#138)
* Update SDK README dep usage (aws#140)
jasdel added a commit that referenced this issue Mar 8, 2018
Services
---
* Synced the V2 SDK with latests AWS service API definitions.

Breaking Changes
---
* `private/mode/api`: Refactor service paginator helpers to iterator pattern (#119)
	* Refactors the generated service client paginators to be an iterator pattern. This pattern improves usability while removing the need for callbacks.
	* See the linked PR for an example.
* `private/model/api`: Removes setter helpers from service API types (#101)
	* Removes the setter helper methods from service API types. Removing clutter and noise from the API type's signature.
	* Based on feedback #81
* `aws`: Rename CanceledErrorCode to ErrCodeRequestCanceled (#131)
	* Renames CanceledErrorCode to correct naming scheme.

SDK Bugs
---
* `internal/awsutil`: Fix DeepEqual to consider string alias type equal to string (#102)
	* Fixes SDK waiters not detecting the correct condition is met. #92
* `aws/external`: Fix EnvConfig misspelled container endpoint path getter (#106)
	* This caused the type to not satisfy the ContainerCredentialsEndpointPathProvider interface.
	* Fixes #105
* `service/s3/s3crypto`: Fix S3Crypto's handling of TagLen (#107)
	* Fixes the S3Crypto's handling of TagLen to only be set if present.
	* V2 Fix for aws/aws-sdk-go#1742
* `private/model/api`: Update SDK service client initialization documentation (#141)
	* Updates the SDK's service initialization doc template to reflect the v2 SDK's configuration update change from v1.
	* Related to #136

SDK Enhancements
---
* `aws`: Improve pagination unit tests (#97)
	* V2 port of aws/aws-sdk-go#1733
* `aws/external`: Add example for shared config and static credential helper (#109)
	* Adds examples for the  config helpers; WithSharedConfigProfile, WithCredentialsValue, WithMFATokenFunc.
* `private/model/api`: Add validation to prevent collision of api defintions (#112)
	* V2 port of aws/aws-sdk-go#1758
* `aws/ec2metadata`: Add support for AWS_EC2_METADATA_DISABLED env var (#128)
	* When this environment variable is set. The SDK's EC2 Metadata Client will not attempt to make requests. All requests made with the EC2 Metadata Client will fail.
	* V2 port of aws/aws-sdk-go#1799
* Add code of conduct (#138)
* Update SDK README dep usage (#140)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

5 participants