-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Include global options using pre-generated rst files #7157
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #7157 +/- ##
===========================================
+ Coverage 92.89% 92.91% +0.02%
===========================================
Files 204 205 +1
Lines 16362 16416 +54
===========================================
+ Hits 15199 15253 +54
Misses 1163 1163
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
It's looking good! I definitely like this approach over all the other ones we considered. This first round of feedback was more around usage/style patterns and interfaces. I did not look at the tests/script or look to deeply through the content generated for the various command types, but I'll look at that in the next pass.
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.
It's looking good. I just had some more comments as we polish this.
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.
Looks good. I think we are getting close. I just had some more comments.
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.
Looks fine. I just had some more comments based on the updates.
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.
Looks good. Just some more more smaller comments mostly around the tests.
44276db
to
5e82043
Compare
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.
Looks good! Let's go ahead squash the commit and reformat the commit message and we should be set to merge this.
e541d42
to
5e82043
Compare
We get a lot of requests from users to include global options in a command's help page. This PR fulfils them by pre-generating .rst files for global options and synopsis and writing them to commands' help docs. An alternative that was considered attempted to plumb the global arg table from the CLIDriver to any help command that would need it. However, we abandoned the approach because it was too invasive to the existing interfaces and there was no easy way to apply the changes to customizations.
5e82043
to
7f9ac3d
Compare
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.
Looks great! 🚢 Merging
[This change in the aws cli](aws/aws-cli#7157) made --help use global_options.rst in the examples folder. We need to keep that file so that running /opt/awscli/aws help doesn't break.
…21716) [This change in the aws cli](aws/aws-cli#7157) made the `help` command use the file `global_options.rst` in the `opt/awscli/awscli/examples` folder. We need to keep that file so that running `/opt/awscli/aws help` doesn't break. I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…21716) [This change in the aws cli](aws/aws-cli#7157) made the `help` command use the file `global_options.rst` in the `opt/awscli/awscli/examples` folder. We need to keep that file so that running `/opt/awscli/aws help` doesn't break. I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 5ce0a09)
…ws#21716) [This change in the aws cli](aws/aws-cli#7157) made the `help` command use the file `global_options.rst` in the `opt/awscli/awscli/examples` folder. We need to keep that file so that running `/opt/awscli/aws help` doesn't break. I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We get a lot of requests from users to include global options in a command's help page. The changes in this PR include:
.rst
files for global options.rst
files are up to datePreview of doc changes