Skip to content
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(pacmak): label "External" stability as "Stable" #1633

Merged
merged 2 commits into from
May 11, 2020

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented May 6, 2020

The distinction between "Stable" (checked stability) and "External"
(unchecked stability) is lost on users, and honestly we shouldn't bother
them with it.

Treat them the same when generating code.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The distinction between "Stable" (checked stability) and "External"
(unchecked stability) is lost on users, and honestly we shouldn't bother
them with it.

Treat them the same when generating code.
@rix0rrr rix0rrr requested a review from a team May 6, 2020 13:23
@rix0rrr rix0rrr self-assigned this May 6, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 6, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 35dc07c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Comment on lines -7 to -9
/// <remarks>
/// <strong>Stability</strong>: Stable
/// </remarks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this now omitted? Sounds like a useful thing to put in the docs, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually makes more sense to me to do the opposite: stable unless indicated otherwise. We already did that for Python and it makes sense.

It's also how every other API is documented. Nobody else just slathers on "stable" everywhere, they might note "deprected" or "experimental" as it applies.

@rix0rrr rix0rrr requested review from eladb and a team May 6, 2020 15:01
@@ -1,4 +1,4 @@
///!MATCH_ERROR: Symbol is re-exported under two distinct submodules (ns1 and ns2)
///!MATCH_ERROR: Symbol is re-exported under two distinct submodules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because on my machine this was breaking the build. The error message I got said (ns2 and ns1) (order different way around), and I did not want to fight with apparent nondeterminism.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhh yeah makes sense!

@rix0rrr rix0rrr requested a review from RomainMuller May 11, 2020 08:22
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: c846778
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr merged commit 8569c00 into master May 11, 2020
@rix0rrr rix0rrr deleted the huijbers/external-as-stable branch May 11, 2020 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants