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

feat(jsii): switch to disable reserved words warnings #1076

Merged
merged 8 commits into from
Dec 18, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Nov 27, 2019

By default, jsii issues a warning when it encounters a symbol that uses a word that is reserved in one of the supported languages. Practically this creates a tremendous amount of noise for large existing projects.

#1073 proposes to add support for a warning exclusion file, which is probably a better long term solution, but in the meantime, this switch allows opting-out of reserved words warnings in order to save the planet.

Tested by adding --disable-reserved-words-warnings to jsii-calc (which uses many reserved words already).


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

By default, jsii issues a warning when it encounters a symbol that uses a word that is reserved in one of the supported languages. Practically this creates a tremendous amount of noise for large existing projects.

#1073 proposes to add support for a warning exclusion file, which is probably a better long term solution, but in the meantime, this switch allows opting-out of reserved words warnings in order to save the planet.

Tested by adding `--disable-reserved-words-warnings` to `jsii-calc` (which uses many reserved words already).
@eladb eladb requested a review from a team as a code owner November 27, 2019 13:34
@eladb eladb requested a review from RomainMuller November 27, 2019 13:34
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@nija-at
Copy link
Contributor

nija-at commented Nov 27, 2019

When would such a warning actually create an issue, and how would we know?

@eladb
Copy link
Contributor Author

eladb commented Nov 27, 2019

@nija-at wrote:

When would such a warning actually create an issue, and how would we know?

Since we have thousands of these warnings right now in the CDK (and jsii), it teaches developers to ignore warnings. We should always operate in "treat warnings as errors" mode.

@nija-at
Copy link
Contributor

nija-at commented Nov 28, 2019

When would such a warning actually create an issue, and how would we know?

Since we have thousands of these warnings right now in the CDK (and jsii), it teaches developers to ignore warnings. We should always operate in "treat warnings as errors" mode.

Clarifying the intention behind my question - If there are hundreds and there are no problems, is this a legit warning?
When would such a warning actually bite us or the customer?
Why should I (as a construct maintainer) care for this error?

@RomainMuller
Copy link
Contributor

@nija-at - the warning is here to prevent adding more identifies that will be awkward to non-TS users. We can usually work around those issues in code generation, but this will often be at the expense of ergonomics... For example, if you call something namespace, you'll force C# users to consistently type @namespace instead...

@nija-at
Copy link
Contributor

nija-at commented Nov 29, 2019

Thanks @RomainMuller - makes sense.

This sounds like bad customer experience we should prevent for new APIs and API parameters. We should add the feature to exclude existing 'warnings' and change their behaviour to be an error (that fails the build). I would even say that this is time critical, to raise our bar on customer experience.

Silencing these warnings for all CDK(v1) is only going to delay this work and allow more of these to-be-avoided identifiers to slip through, isn't it?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

I like this so much more than the previous iteration. Only one minor comment on there, and we're good to go!

packages/jsii/lib/warnings.ts Outdated Show resolved Hide resolved
packages/jsii/bin/jsii.ts Outdated Show resolved Hide resolved
packages/jsii/bin/jsii.ts Show resolved Hide resolved
Elad Ben-Israel and others added 3 commits December 18, 2019 14:11
@eladb eladb requested a review from RomainMuller December 18, 2019 12:12
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • 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 Dec 18, 2019

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Dec 18, 2019
@mergify mergify bot merged commit 5389def into master Dec 18, 2019
@mergify mergify bot deleted the benisrae/disable-reserved-words branch December 18, 2019 12:35
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants