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-template): [button-has-type] add rule #928

Merged

Conversation

chernodub
Copy link
Contributor

closes #823

@chernodub
Copy link
Contributor Author

Hi @JamesHenry, I'd appreciate if you could look into this PR. So that we could continue working on it or, at least, close it. Thanks!

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.

I'm very sorry for the delay in getting back to you on this @chernodub and thank you so much for taking the initiative and contributing this rule.

Please take a look at the review comments. You will also need to run yarn update-rule-docs in order to generate the necessary rule documentation to accompany this rule.

@JamesHenry JamesHenry added awaiting response enhancement New feature or request labels Jun 11, 2022
@chernodub chernodub force-pushed the feat/template/button-has-type branch from f095b34 to cd5386e Compare June 11, 2022 22:03
@chernodub
Copy link
Contributor Author

Thanks a lot for a valuable feedback @JamesHenry! 🙏

I've updated the code following your comments (agreed to all the points you mentioned). I've also rebased the changes on origin/master branch, so it's ready for merging.

@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #928 (cd5386e) into master (0bda2bb) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
+ Coverage   87.20%   87.35%   +0.15%     
==========================================
  Files         147      149       +2     
  Lines        2782     2815      +33     
  Branches      450      454       +4     
==========================================
+ Hits         2426     2459      +33     
  Misses        252      252              
  Partials      104      104              
Flag Coverage Δ
unittest 87.35% <100.00%> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
packages/eslint-plugin-template/src/index.ts 66.66% <ø> (+0.70%) ⬆️
...slint-plugin-template/src/rules/button-has-type.ts 100.00% <100.00%> (ø)
...ugin-template/tests/rules/button-has-type/cases.ts 100.00% <100.00%> (ø)

@JamesHenry JamesHenry merged commit f19bb30 into angular-eslint:master Jun 12, 2022
@JamesHenry
Copy link
Member

Thanks a lot @chernodub! I'll release this on v13 so you're not forced to upgrade to v14 of Angular to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants