-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Update docs #2
Update docs #2
Conversation
packages/docs/src/conf.py
Outdated
@@ -22,6 +22,10 @@ | |||
# | |||
# AWS Sphinx configuration file. | |||
# | |||
# For more information about how to configure this file, see: |
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.
Revert
packages/docs/src/cloudformation.rst
Outdated
|
||
.. _multiple_environments: | ||
|
||
Creating Multiple Environments |
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.
Why is this under cloudformation?
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.
Good question. Don't even know where this came from, this is confusing because it's not even implemented yet.
packages/docs/src/concepts.rst
Outdated
because in most cases the |cdk| initializes new | ||
constructs from within the context of their parent construct, | ||
so |this| represents the parent. | ||
In almost call cases, you will want to pass the keyword ``this`` for the ``parent`` |
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.
"call" => "all"
packages/docs/src/concepts.rst
Outdated
@@ -50,26 +50,32 @@ to instantiate the ``StorageLayer`` construct. | |||
|
|||
When you initialize a construct, | |||
add the construct to the construct tree by specifying the parent construct as the first initializer parameter, | |||
a unique (to your stack) identifier for the construct as the second parameter, | |||
and an optional set of properties for the final parameter, | |||
a unique identifier for the construct as the second parameter, |
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.
We should mention that it needs to be unique amongst siblings and not globally
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.
Agreed about removing the globally part as it's wrong.
Don't necessarily agree on adding the qualifier on the uniqueness here. Reasons:
- It makes the sentence longer and harder to read
- The constraint is quite logical and can go without saying. This is the same as declaring variables, basically, which people are already used to.
- The constraint is also mentioned below under "construct names", which will be the more canonical location to put all reference information on construct names.
- The error message you get when you do it wrong is very immediate and descriptive.
All in all, very low probability of a user doing it wrong and the impact is negligible, so no need for handholding on this I think.
packages/docs/src/cloudformation.rst
Outdated
Using |level1| Constructs | ||
################### | ||
|
||
|level1| constructs are all constructs found in the :py:mod:`aws-cdk-resources` |
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.
Sentence feels convoluted "level1 constructs are all constructs found...."
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.
"Are those constructs found..." ? Or just "are found..." ?
packages/docs/src/cloudformation.rst
Outdated
|
||
In general, you shouldn't need to use this type of Constructs, unless you have | ||
special requirements or there is no |level2| package for the AWS resource you | ||
need yet. Prefer using other packages with higher-level constructs instead. See |
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.
"Prefer to use the AWS Construct Library"
I would recommend to stop use the replacements |level1|
and |level2|
because it will make reviewing these docs much easer. I think settled on: AWS Construct Library (or just AWS Constructs) (for L2) and CloudFormation Resource Constructs (or just CloudFormation Resources) for (L1s)
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 actually completely agree. It also allows us to leave off the constant "AWS" qualifier, which is technically correct but makes every usage of the term horribly long and convoluted to read.
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.
Oh by the way, that IS different terminology than we were using thus far. We were using
- AWS Resource Construct
- AWS Construct Library Construct
I do think I like yours better, so I'll do a replace on them.
- Changed wording, add more references between sections. - Highlighted some details better. - Reorganized discussion about L1s and L2s a bit to make it flow better. - Changes the title of the section that used to be called "using l2s" into "writing CDK apps". - Update formatting. - Get rid of level1, level2 substitutions
252c502
to
34a3dad
Compare
I force-pushed a squash, so it looks like nothing happened but actually the diff got updated. |
Attempt #2 our diff command represents the URLSuffix (typically amazonaws.com) by using the CloudFormation pseudo-parameter. modifies the principal in the test to expect it instead. ran tests locally. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
# This is the 1st commit message: Add Identity Pool construct # This is the commit message #2: Bug fixes # This is the commit message #3: Bug fixes # This is the commit message #4: Formatting # This is the commit message #5: Add construct methods # This is the commit message #6: Remove flat # This is the commit message #7: Fix issues
into "writing CDK apps".