-
Notifications
You must be signed in to change notification settings - Fork 55
chore: add documentation and example #90
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
Conversation
kggilmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to advertise that we'll provide other services upon request without knowing more about how we'll manage that. It may be worth bringing Vishaal and Vinod into the conversation, as it's more of a policy than a technical concern.
docs/GettingStarted.md
Outdated
|
|
||
| ## Giving Feedback | ||
|
|
||
| * Submit [issues](https://github.com/awslabs/aws-sdk-kotlin/issues) - this is the preferred channel to interact with our team No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we want to add the slack channel here as well? Sometimes people have questions but don't want to commit to creating an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was hesitant to add a private slack channel but yeah we can
| ### Features | ||
| * Coroutine API | ||
| * DSL Builders | ||
| * Default (environment or config) or static credential providers only. Additional providers will be added in later releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is CDO instincts but would seem better just to mention what's provided and not allude to what/when will come next, for credential providers and platform support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but we don't have a clean public/internal roadmap yet so I think trying to provide a bit about what's missing to head off questions might be in order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable. The platform support line (one below) I guess is the one I'm concerned with. Maybe rewording to imply what platforms are supported without giving hints as to when other platform support will come would be a better way to manage expectations. Something like Kotlin platforms supported in this release: JVM
|
|
||
| ### Services in this release | ||
|
|
||
| * DynamoDB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question
Perhaps it's forthcoming but is there some build file which is specifying these services go into a zip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our internal gitfarm repo has more info
|
The additional documentation is well written, concise, and informative. Nice work! |
| println("2013 film titles:") | ||
| println(titles) | ||
|
|
||
| }catch(ex: AwsServiceException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
Seems like these files should be run through the linter..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do, I don't really want these in the aws-sdk-kotlin repo but I don't have a better place to put them at the moment.
| import aws.sdk.kotlin.runtime.AwsServiceException | ||
| import kotlinx.coroutines.delay | ||
| import kotlinx.coroutines.runBlocking | ||
| import com.google.gson.JsonElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
there may be suitable KMP JSON parsers we could use here, with the benefit being when we are ready to test or provide for eval on JS or native, the example code should work unaltered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well ideally we provide such an API so that users don't need to add another dependency: smithy-lang/smithy-kotlin#243
settings.gradle.kts
Outdated
| */ | ||
|
|
||
| pluginManagement { | ||
| // FIXME: temporary workaround bintray fiasco, see: https://github.com/Kotlin/dokka/issues/1779 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kamil says he fixed it 5 hrs ago FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is all that seemed to work. I can retry it
kggilmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.