-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: rename core classes adding a Cfn prefix #1960
Conversation
There's also a bunch of other classes that were not mentioned in the #1462 issue, but perhaps should be renamed as well?
Let me know should those be included as well. |
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 don't really have L2 surface for most of these things... Are we positive users don't routinely have to interact with those classes?
CfnCondition
, CfnElement
and CfnResource
should 100% be the names though - my concern really is around Output
and Parameter
there.
0b74b99
to
8618108
Compare
Addressed the 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 good to me.
Yes, good call about other renames:
Rule
toCfnRule
Mapping
toCfnMapping
.Referenceable
maybe toCfnRefElement
(it's aCfnElement
that can beRef
ed).IConditionExpression
=>ICfnConditionExpression
I wouldn't touch Include
for now, if at all, it should be IncludeTemplate
or IncludeCfnTemplate
.
The "rule of thumb" is that we use the Cfn
prefix only with types that represent CloudFormation template entities.
Also, CfnReference
should be Reference
(it's not only for CloudFormation refs anymore).
And please flatten the file tree while you are wrecking havoc in this library.
@RomainMuller wrote:
I see your point about |
8618108
to
21a2034
Compare
Done.
Done.
Done.
Done.
Left alone.
Done.
Done. |
Renamed |
4d1884a
to
bd842a6
Compare
Rebased on top of Sam's changes to the AWS Glue package. |
Nice! Thanks for picking this one up @skinny85 ! |
My pleasure :) |
To better align with our L1 layer that is closely tied to CloudFormation, add the
Cfn
prefix to classes from the@aws-cdk/cdk
package.BREAKING CHANGE: rename Output to CfnOutput.
BREAKING CHANGE: rename Condition to CfnCondition.
BREAKING CHANGE: rename StackElement to CfnElement.
BREAKING CHANGE: rename Parameter to CfnParameter.
BREAKING CHANGE: rename Resource to CfnResource.
Fixes #1462
Fixes #288
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.