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(eslint-plugin): add prefer-standalone rule for checking all components, directives and pipes #1627

Merged
merged 1 commit into from Mar 15, 2024

Conversation

csvn
Copy link
Contributor

@csvn csvn commented Nov 19, 2023

Adds new rules for checking @Directive and @Pipe for being standalone as a companion to @Component being checked by the existing rule.

An alternative would be to add options to the current prefer-standalone-component rule to allow checking pipes/directives too, but I was afraid it might be confusing with the naming of the existing rule to also apply to directives/pipes. Let me know if think it would be better to merge the code into a single rule (it would likely be less code duplication), or refactor common code into utility functions maybe.

@csvn csvn force-pushed the feat/rule-prefer-standalone branch 2 times, most recently from 05f3885 to 57196ce Compare November 19, 2023 22:52
Copy link

nx-cloud bot commented Nov 30, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 57196ce. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c0d8a6e) 89.56% compared to head (ee4ebf2) 89.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1627      +/-   ##
==========================================
+ Coverage   89.56%   89.59%   +0.02%     
==========================================
  Files         171      173       +2     
  Lines        3202     3229      +27     
  Branches      544      548       +4     
==========================================
+ Hits         2868     2893      +25     
- Misses        198      199       +1     
- Partials      136      137       +1     
Flag Coverage Δ
unittest 89.59% <92.30%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/eslint-plugin/src/index.ts 71.18% <ø> (+0.49%) ⬆️
...nt-plugin/src/rules/prefer-standalone-component.ts 89.47% <ø> (ø)
...lint-plugin/tests/rules/prefer-standalone/cases.ts 100.00% <100.00%> (ø)
...kages/eslint-plugin/src/rules/prefer-standalone.ts 90.00% <90.00%> (ø)

Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Hi @csvn I'm sorry for the delay in getting back to you on this one.

I think what might be best overall here is to create a new rule: prefer-standalone which applies checks for all the things (as really people should either be doing all or nothing I think).

At the same time we deprecate the prefer-standalone-component rule and look to remove it in the next major for clarity

@csvn
Copy link
Contributor Author

csvn commented Jan 9, 2024

I think that's a good idea. I'll update this MR soon based on that feedback.

@csvn csvn force-pushed the feat/rule-prefer-standalone branch from 57196ce to ee4ebf2 Compare February 7, 2024 12:59
Copy link

nx-cloud bot commented Feb 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ee4ebf2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

@csvn csvn changed the title feat(eslint-plugin): add rules for standalone pipes and directives feat(eslint-plugin): add prefer-standalone rule for checking all directives Feb 7, 2024
Comment on lines +32 to +33
const standaloneRuleFactory =
(type: DecoratorTypes) => (node: TSESTree.Decorator) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contents of this function is the same as for prefer-standalone-component, with the addition of turning it into a factory function and passing in the decorator type for the messageID data property.


const messageId: MessageIds = 'preferStandalone';

export const valid = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The valid & invalid test cases here are the same as for prefer-standalone-component + new cases for @Pipe and @Directive (taken from prior commits).

@csvn
Copy link
Contributor Author

csvn commented Feb 7, 2024

@JamesHenry I think I've addressed your feedback now, and that this PR is ready for review. Sorry for it taking a while for me to get back to this one.

@csvn csvn requested a review from JamesHenry February 7, 2024 13:08
@JamesHenry JamesHenry changed the title feat(eslint-plugin): add prefer-standalone rule for checking all directives feat(eslint-plugin): add prefer-standalone rule for checking all components, directives and pipes Mar 15, 2024
@JamesHenry JamesHenry merged commit e013477 into angular-eslint:main Mar 15, 2024
9 checks passed
@JamesHenry
Copy link
Member

Thanks so much @csvn!

@csvn csvn deleted the feat/rule-prefer-standalone branch March 19, 2024 08:47
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.

None yet

2 participants