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

chore: ban @experimental #14070

Merged
merged 9 commits into from
Apr 21, 2021
Merged

chore: ban @experimental #14070

merged 9 commits into from
Apr 21, 2021

Conversation

NetaNir
Copy link
Contributor

@NetaNir NetaNir commented Apr 9, 2021

Add a lint rule that disallow the use of @experimental in stable module. Ideally we would do the same for experimental modules, but for these, jsii marks all properties as experimental. Since we only care about stable modules, this is sufficient. Im open to other suggestion as well.

Should only be merged after #14071 which removes all experimental API.


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

@gitpod-io
Copy link

gitpod-io bot commented Apr 9, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 9, 2021
@NetaNir NetaNir marked this pull request as draft April 9, 2021 07:13
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Nice! Only minor comments.

packages/awslint/lib/rules/docs.ts Outdated Show resolved Hide resolved
packages/awslint/lib/rules/docs.ts Outdated Show resolved Hide resolved
packages/awslint/lib/rules/docs.ts Outdated Show resolved Hide resolved
@@ -71,6 +72,21 @@ docsLinter.add({
},
});

docsLinter.add({
code: 'no-experimental-apis',
message: 'The use of @experimental in not allowed (either directly or via parent class)',
Copy link
Contributor

Choose a reason for hiding this comment

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

(either directly or via parent class)

what's this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it, I agree it does not add value to the error. What I was referring to is the fact that if a parent class will add a @experimental annotation, the error will be thrown for every child class as well as the parent, regardless of whether the inherited property have the annotation. But since we don't allow the annotation at all, it will not be on the parent class.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

@RomainMuller we should add support in JSII to disallow experimental APIs, no?

@NetaNir
Copy link
Contributor Author

NetaNir commented Apr 12, 2021

@eladb I don't think we need to remove the @experimental annotation in jsii. The decision to remove experimental APIs is an AWS CDK specific decision, and the @experimental annotation is a jsii public feature that might be useful for other jsii customers. Additionally, wouldn't removing it is will breaking change in jsii?

@eladb
Copy link
Contributor

eladb commented Apr 12, 2021

Agreed. I just think that it will be useful to add a jsii compiler/config flag to disallow experimental.

NetaNir and others added 4 commits April 14, 2021 22:03
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
@NetaNir NetaNir marked this pull request as ready for review April 15, 2021 08:15
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Apr 15, 2021
@NetaNir NetaNir removed the pr/do-not-merge This PR should not be merged at this time. label Apr 19, 2021
mergify bot pushed a commit that referenced this pull request Apr 20, 2021
As a part of v2 efforts we are stabilizing all `@experimental` APIs from stable modules.  A separate PR #14070, adds a lint rule that ban the usage of @experimental.

#### Note on experimental modules
The removal of the @experimental annotation from APIs in experimental modules does not change their stability, as it determined only by their containing module stability.  

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 4c5d1cf
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Apr 21, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 283ed02 into master Apr 21, 2021
@mergify mergify bot deleted the neta/remove-exp-apis branch April 21, 2021 00:21
john-tipper pushed a commit to john-tipper/aws-cdk that referenced this pull request May 10, 2021
As a part of v2 efforts we are stabilizing all `@experimental` APIs from stable modules.  A separate PR aws#14070, adds a lint rule that ban the usage of @experimental.

#### Note on experimental modules
The removal of the @experimental annotation from APIs in experimental modules does not change their stability, as it determined only by their containing module stability.  

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
john-tipper pushed a commit to john-tipper/aws-cdk that referenced this pull request May 10, 2021
Add a lint rule that disallow the use of @experimental in stable module. Ideally we would do the same for experimental modules, but for these, jsii marks all properties as experimental. Since we only care about stable modules, this is sufficient. Im open to other suggestion as well. 

Should only be merged after aws#14071 which removes all experimental API.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
As a part of v2 efforts we are stabilizing all `@experimental` APIs from stable modules.  A separate PR aws#14070, adds a lint rule that ban the usage of @experimental.

#### Note on experimental modules
The removal of the @experimental annotation from APIs in experimental modules does not change their stability, as it determined only by their containing module stability.  

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
Add a lint rule that disallow the use of @experimental in stable module. Ideally we would do the same for experimental modules, but for these, jsii marks all properties as experimental. Since we only care about stable modules, this is sufficient. Im open to other suggestion as well. 

Should only be merged after aws#14071 which removes all experimental API.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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