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): new rules for reference interfaces #27790

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Nov 1, 2023

Introducing two new rules, and immediately accepting all current violations.

  • resource-interface-extends-resource-ref - all resources interfaces like IBucket must also implement the reference interface, e.g. ICfnBucket
  • ref-via-ref-interface - all APIs should accept the reference interface ICfnBucket over the resource interface IBucket

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

@aws-cdk-automation aws-cdk-automation requested a review from a team November 1, 2023 11:21
@github-actions github-actions bot added the p2 label Nov 1, 2023
@mrgrain mrgrain changed the title Mrgrain/chore/awslint chore: linter rules for reference interfaces Nov 1, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 1, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 1, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 1, 2023 20:10

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mrgrain mrgrain force-pushed the mrgrain/chore/awslint branch 4 times, most recently from ebbaa51 to fade49e Compare November 2, 2023 17:19
@mrgrain mrgrain marked this pull request as ready for review November 2, 2023 17:23
e.assert(resourceInterface.extends(interfaceBase), resourceInterface.fqn);
},
});

/*
// This rule is the worst
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 sure is!

`If this is intentional, add "${EXCLUDE_ANNOTATION_REF_VIA_INTERFACE}" to element's jsdoc`,
eval: e => {
eval: (e) => {
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 rule hasn't changed. I just refactored the code used to visit spec nodes to be reusable.

Comment on lines +6 to +10
const EXCLUDE_ANNOTATION_REF_VIA_INTERFACE =
'[disable-awslint:ref-via-interface]';

const EXCLUDE_ANNOTATION_REF_VIA_REF_INTERFACE =
'[disable-awslint:ref-via-ref-interface]';
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 considered wrapping them both into a single rule, but found it too confusing and complicated.
Now we have two rules:

  • ref-via-interface guarantees that we are not expecting concrete instances of constructs
  • ref-via-ref-interface guarantees that we prefer ICfnWhatever reference interfaces over IWhatever interfaces

@mrgrain mrgrain changed the title chore: linter rules for reference interfaces chore(awslint): new rules for reference interfaces Nov 2, 2023
@mikewrighton
Copy link
Contributor

Generally looks good to me - did you see what impact this had on the lint times?

@mrgrain
Copy link
Contributor Author

mrgrain commented Nov 3, 2023

Generally looks good to me - did you see what impact this had on the lint times?

I'll check that.

@mrgrain
Copy link
Contributor Author

mrgrain commented Nov 7, 2023

Adds about 5s to awslint. Deeming this as acceptable. I am working on some speed-ups for the linter.

@mrgrain mrgrain merged commit c990c00 into conroy/generate Nov 7, 2023
10 checks passed
@mrgrain mrgrain deleted the mrgrain/chore/awslint branch November 7, 2023 11:29
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. p2 pr/needs-cli-test-run This PR needs CLI tests run against it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants