Improve comments and add a doc.go #1515
Conversation
There was a problem hiding this comment.
Looks good, really great to have the extra docs and examples.
In general I suggest taking an additional pass over some of the wording. In several cases a Users is referenced. In this case since the reader is the audience I'd suggest using you, or reword to be declarative without referencing the audience directly.
Also a couple cases of future tense vs precent mentioned below.
| // This example scans the entire Music table, and then narrows the results to songs | ||
| // by the artist "No One You Know". For each item, only the album title and song title | ||
| // are returned. | ||
| func ExampleBuilder_WithFilter_shared00() { |
There was a problem hiding this comment.
The _shared00 part isn't needed for these docs.
service/dynamodb/expression/doc.go
Outdated
| accordingly. For example, ConditionBuilder represents a DynamoDB Condition | ||
| Expression, an UpdateBuilder represents a DynamoDB Update Expression, and so | ||
| forth. | ||
| The following example will show a sample ConditionExpression and how to build an |
There was a problem hiding this comment.
future tense :)
`The following is an example using a condition builder to create the expression where DynamoDB record's field name "Artist" equals the value "No One You Know".
service/dynamodb/expression/doc.go
Outdated
| condExpr := "Artist = :a" | ||
| condBuilder := expression.Name("Artist").Equal(expression.Value("No One You Know")) | ||
|
|
||
| In order to retrieve the formatted DynamoDB Expression strings, users must call |
There was a problem hiding this comment.
nit, audience is the reader, not a third party the reader will instruct on how to use the library.
In this case can replace users must call, with just call the.
| // WithFilter method adds the argument ConditionBuilder as a Filter Expression | ||
| // to the argument Builder. If the argument Builder already has a | ||
| // ConditionBuilder representing a Filter Expression, WithFilter() | ||
| // overwrites the existing ConditionBuilder. Users are able to add other |
There was a problem hiding this comment.
target you, instead of Users. Or consider reword to more declarative such as, Additional expressions can be added to the Builder via the With___ methods. To build an Expression, call Build.
service/dynamodb/expression/doc.go
Outdated
| TableName: aws.String("Music"), | ||
| } | ||
|
|
||
| Users must always fill in the ExpressionAttributeNames and |
| } | ||
|
|
||
|
|
||
| #### Idiosyncrasies of the Expression Package |
There was a problem hiding this comment.
Not really sure this section adds very much value to the user to the documentation.
To explain expression specific requirements/expectations would be better in the Builder types themselves.
| @@ -0,0 +1,94 @@ | |||
| # expression | |||
There was a problem hiding this comment.
This file's content should be apart of the doc.go file . This file as it is will never be visible to users when using godoc, or similar tools.
A new file added to the https://github.com/awsdocs/aws-go-developer-guide repo would be a good place to put extra higher level documentation, concepts, and detailed example based information. But About DynamoDB Expressions and Using the Package sections should be in the doc.go file regardless.
For changes to files under the
/model/folder, and manual edits to autogenerated code (e.g./service/s3/api.go) please create an Issue instead of a PR for those type of changes.If there is an existing bug or feature this PR is answers please reference it here.