-
Notifications
You must be signed in to change notification settings - Fork 55
service protocol build control #121
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
…ity for inclusion and exclusion based on protocol.
ianbotsf
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.
Very nice.
aajtodd
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.
Nice! Couple suggestions but LGTM.
I wrote this unit test to verify functionality but didn't include it since it's build code. Unsure of where tests would go for that.
@ianbotsf @kggilmer Tagging both of you for visibility on Gradle functionality...
If you wanted to you could abstract this away into buildSrc which would allow tests. buildSrc allows custom code that is compiled BEFORE gradle configures the project. All types/functions/tasks/etc in buildSrc are made available to every gradle build script that is part of the project. People use this all the time to define library versions or custom tasks they want re-used, etc. It can also be used to define custom plugins (although more and more people are using composite builds for custom plugins).
Example from toolkits repo: https://github.com/aws/aws-toolkit-jetbrains/tree/master/buildSrc
Example from kotlinx.coroutines repo: https://github.com/Kotlin/kotlinx.coroutines/tree/master/buildSrc
|
|
||
| ```sh | ||
| ./gradlew -Paws.services=lambda :codegen:sdk:bootstrap | ||
| ./gradlew -Paws.services=+lambda :codegen:sdk:bootstrap |
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
should we retain + as optional? It would maintain the current behavior and make e.g. generating a handful of services less ceremonious aws.services=+lambda,+s3,+iam
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.
Yeah I was thinking about that too. I went back and forth but slightly favor requiring requiring the plus for these reasons:
- generally speaking less configuration variance is easier to maintain over time (very weak admittedly in this case)
- the
+self-documents the feature that exclusions are also available. - it may be less confusing when reasoning about the rules if the ex/in-lusion is explicit.
IMHO these are a bit stronger than the convenience argument
README.md
Outdated
| Included services require a '+' character prefix and excluded services require a '-' character. | ||
| If any items are specified for inclusion, only specified included members will be generated. If no items | ||
| are specified for inclusion, all members not excluded will be generated. | ||
| When unspecified all services provided by models are generated. |
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.
suggestion
When unspecified all services found in the directory specified by the `modelsDir` property ...
README.md
Outdated
|
|
||
| Some example entries for `local.properties`: | ||
| ```ini | ||
| # Example ~ Generate only AWS Lambda: |
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
Example ~ prefix on all these seems redundant
| NOTE: You currently need an HTTP server to view the documentation in browser locally. You can either use the builtin server in Intellij or use your favorite local server (e.g. `python3 -m http.server`). See [Kotlin/dokka#1795](https://github.com/Kotlin/dokka/issues/1795) | ||
|
|
||
| ### Build properties | ||
| ### Build Properties |
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.
fix
The intent of this section was to document the available build properties. The new section you added with the examples is fine and can remain as is but I still think we ought to document the available properties in one place.
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.
Sure, I added a table that describes all available properties directly in this section.
|
Regarding the |
Issue #, if available: smithy-lang/smithy-kotlin#306
Description of changes:
Testing Done
I wrote this unit test to verify functionality but didn't include it since it's build code. Unsure of where tests would go for that.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.