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: awslint #1468

Merged
merged 39 commits into from
Jan 3, 2019
Merged

chore: awslint #1468

merged 39 commits into from
Jan 3, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jan 2, 2019

Introducing awslint: a linter for the AWS Construct Library APIs. The goal of the linter is to enforce the API design guidelines across the AWS Construct Library.

For details on how to use the linter see README. All rules are going to be documented under GUIDELINES.

  • The awslint npm script has been added to all modules through pkglint.
  • All violations have been captured through "exclude"s in package.json files so we can continue to lint from here.

TODO:

  • Finish writing all guidelines
  • Add awslint to build

This change is built on top of #1444


Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

Elad Ben-Israel added 25 commits December 26, 2018 19:46
Initial RFC for design of #1432
Adds CloudFormation resource metadata which enables tools such as SAM
CLI to find local assets used by resources in the template.

See design document under [design/code-asset-metadata.md](./design/code-asset-metadata.md)

Fixes #1432
- Move all construct members (besides protected "validate") under "node"
- Rename "parent" to "scope"
- Rename "id" or "name" to "scid" (not sure about this one)
WIP

Introducing "awslint": a linter for the AWS Construct Library.
@eladb eladb requested a review from rix0rrr January 2, 2019 13:39
@eladb eladb changed the title chore: awslint [WIP] chore: awslint Jan 2, 2019
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/lib/rules/construct.ts Show resolved Hide resolved
tools/awslint/lib/rules/resource.ts Outdated Show resolved Hide resolved
@eladb eladb changed the base branch from benisrae/construct-refactors to master January 3, 2019 12:00
@eladb eladb changed the title [WIP] chore: awslint chore: awslint Jan 3, 2019
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
}
```

The reason we are defining `export` on the resource interface and not on the resource
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worthwhile splitting this document into two parts:

  1. These are the rules.
  2. This is why the rules are what they are.

The reason is if I'm implementing something and I want to quickly validate whether I'm following the rules properly, I just want to be able to skim them and not have to skip swathes of rationale.

The rationale can probably go at the end or into a different file, it's going to be mostly interesting if people are curious or want to quibble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think awslint list is not good enough?

$ awslint list
module-name: module name must be @aws-cdk/aws-<namespace>
construct-ctor: signature of all construct constructors should be "scope, id, props"
resource-class: every resource must have a resource class (L2)
resource-class-is-construct: resource classes must extend "cdk.Construct" directly or indirectly
resource-props: an interface for resource props must be defined
resource-interface: every resource must have a resource interface
import-props-interface: every resource must have an "FooImportProps" interface
resource-interface-extends-construct: resource interface must extends cdk.IConstruct
resource-attribute: resources must represent all attributes as properties
resource-attribute-immutable: resource attributes must be immutable (readonly)
import: resource class must have a static "import" method
export: resource interface must have an "export" method

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's tension between "rule caption", "rule + short description + example" and "rule + rationale".

But also I don't want to create duplicate work for nothing. If we say this doc is the rationale doc, I think I can live with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the rationale doc, but we can definitely add a longer description to the code itself.

tools/awslint/GUIDELINES.md Outdated Show resolved Hide resolved
Elad Ben-Israel added 4 commits January 3, 2019 16:35
- moved guidelines to design/aws-guidelines.md
- added a section about awslint in the contribution guide
- added support for auto-compiling jsii if .jsii is not there
- add "awslint list" to contributing.md
- updates to guidelines based on cr feedback
- allow "awslint list" to be executed anywhere
- lint constructor-ctor only if non-abstract
- fix typo
@eladb eladb merged commit 9f45783 into master Jan 3, 2019
@eladb eladb deleted the benisrae/awslint branch January 3, 2019 20:16
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

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.

3 participants