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.

Sign up for GitHub

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

Jump to bottom

fix(utils): support passing data and suggestions individually for each error #491

Merged
merged 28 commits into from May 22, 2021
Merged

fix(utils): support passing data and suggestions individually for each error #491

merged 28 commits into from May 22, 2021

Conversation

rafaelss95
Copy link
Member

@rafaelss95 rafaelss95 commented May 20, 2021 •
edited

With this PR we can now test data and suggestions individually for each error. Now, the test fail if we try to test something that uses data properties without passing data.

In addition, it improves the types, making it mandatory and mutually exclusive to pass one of the two properties: messageId or messages, so there is no longer a need to throw a compile-time error.

@rafaelss95 rafaelss95 changed the title fix(utils): support passing data and suggestions individually for each error fix(utils): support passing data and suggestions individually for each error May 20, 2021
@rafaelss95 rafaelss95 force-pushed the fix/utils-convertAnnotatedSourceToFailureCase branch from 02ba534 to 7cbecb4 Compare May 20, 2021 00:09
@nx-cloud
Copy link

nx-cloud bot commented May 20, 2021 •
edited

Nx Cloud Report

CI ran the following commands for commit e6aa98e. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=build --all --parallel
#000000 nx run-many --target=check-configs --all --parallel
#000000 nx run-many --target=test --all --parallel
#000000 nx run-many --target=typecheck --all --parallel

Sent with 💌 from NxCloud.

@rafaelss95 rafaelss95 force-pushed the fix/utils-convertAnnotatedSourceToFailureCase branch 8 times, most recently from 82308a6 to 53e4915 Compare May 20, 2021 03:55
JamesHenry
JamesHenry requested changes May 21, 2021
View reviewed changes
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.

@rafaelss95 Thanks for working on this, but you have mixed in a number of subjective and unrelated changes (particularly to rule messages) to the core purpose of the PR.

Please revert them, and if you feel strongly about them, apply them in other PRs because it is impractical to review this in its current form

@rafaelss95
Copy link
Member Author

@JamesHenry Ok, let me try to explain why there are so many changes in this PR...

When I changed the implementation of convertAnnotatedSourceToFailureCase to support data (useful atm for no-duplicate-attributes and cyclomatic-complexity - changes in this PR -) and suggestions (useful for #486 and #489 (now that you mentioned there)) individually, the data and suggestions have become mandatory, so in any case this PR will have to touch at least the rules test files.

So when I started to fill the data property in tests, I realized that in addition to having some unnecessary information present in the messages that are really exhausting to fill like {{className}} for each report (previously discussed in mgechev/codelyzer#778 (comment)), there was a lot that could be improved in rules that deals with prefix/suffix using toHumanReadableText for example.

I tried to separate the changes to the maximum in each rule by the commits to facilitate the review proccess, but it seems that it isn't easy yet 😞 .

I confess that thought about not messing with the non .test.ts files, but then I thought it would be redundant to just add data and suggestions to this PR and then send pull requests in sequence changing everything again 🤷‍♂️ .

That being said, do you still think that we should revert all changes in non .test.ts files and create PRs changing the rules' messages and tests?

This was referenced May 21, 2021
Merged
Merged
@JamesHenry
Copy link
Member

@rafaelss95 I definitely think it's a good change overall, but it does mean we have to discuss a few changes to those messages before this can be merged. Based on what you have said I will do that here

JamesHenry
JamesHenry requested changes May 22, 2021
View reviewed changes
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.

Many thanks!

packages/eslint-plugin/src/rules/component-class-suffix.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/component-selector.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/contextual-lifecycle.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/directive-selector.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/directive-class-suffix.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-input-rename.ts Outdated Show resolved Hide resolved
@rafaelss95 rafaelss95 force-pushed the fix/utils-convertAnnotatedSourceToFailureCase branch from a701819 to 13413ad Compare May 22, 2021 15:55
@rafaelss95
fix(utils): support passing data and suggestions individually for…
61c72c5 
… each error
@rafaelss95
test(eslint-plugin-template): [accessibility-alt-text] add missing `d…
714d10f 
…ata`
@rafaelss95
test(eslint-plugin-template): [accessibility-valid-aria] add missing …
99e1121 
…`data`
@rafaelss95
test(eslint-plugin-template): [cyclomatic-complexity] add missing data
587acc2
@rafaelss95
test(eslint-plugin-template): [no-distracting-elements] add missing `…
83d6d04 
…data`
@rafaelss95
test(eslint-plugin-template): [no-duplicate-attributes] add missing `…
8340089 
…data`
@rafaelss95
refactor(eslint-plugin): use toHumanReadableText to make messages u…
6b690c2 
…ser-friendly
@rafaelss95
test(eslint-plugin): [component-selector] add missing data
7bd9270
@rafaelss95
test(eslint-plugin): [component-class-suffix] add missing data
db87799
@rafaelss95
test(eslint-plugin): [contextual-decorator] add missing data
b5438a3
@rafaelss95
fix(eslint-plugin): [contextual-lifecycle] typo in messageId
d5847fd
@rafaelss95
test(eslint-plugin): [contextual-lifecycle] add missing data
53f64c8
@rafaelss95
test(eslint-plugin): [directive-class-suffix] add missing data
30a29c4
@rafaelss95
test(eslint-plugin): [directive-selector] add missing data
7ca2369
@rafaelss95
test(eslint-plugin): [no-input-prefix] add missing data
22fdc33
@rafaelss95
test(eslint-plugin): [no-input-rename] remove unnecessary data
9b1b5d5
@rafaelss95
test(eslint-plugin): [pipe-prefix] add missing data
f022a38
@rafaelss95
test(eslint-plugin): [use-component-selector] remove unnecessary data
104158a
@rafaelss95
test(eslint-plugin): [use-lifecycle-interface] add missing data
535f7a7
@rafaelss95
fixup! test(eslint-plugin): [contextual-lifecycle] add missing data
28a8517
@rafaelss95
fixup! test(eslint-plugin): [component-class-suffix] add missing data
edbd28d
@rafaelss95
fixup! test(eslint-plugin): [directive-class-suffix] add missing data
f77cd5d
@rafaelss95
fixup! test(eslint-plugin): [component-selector] add missing data
f049698
@rafaelss95
fixup! test(eslint-plugin): [directive-selector] add missing data
cac7bcb
@rafaelss95
refactor: add missing style guide link
1f06f34
@rafaelss95
refactor: remove extraneous quotes from numbers
94a64fd
@rafaelss95 rafaelss95 force-pushed the fix/utils-convertAnnotatedSourceToFailureCase branch from 13413ad to 94a64fd Compare May 22, 2021 15:57
@JamesHenry
Copy link
Member

Completed another full pass and left effectively the same comment twice

@JamesHenry
Copy link
Member

LGTM, anything more to be done on this branch @rafaelss95?

@rafaelss95
Copy link
Member Author

LGTM, anything more to be done on this branch @rafaelss95?

I think we're done here 🚀.

@JamesHenry JamesHenry merged commit 70b01bd into angular-eslint:master May 22, 2021
@JamesHenry
Copy link
Member

Great, thanks!

@rafaelss95 rafaelss95 deleted the fix/utils-convertAnnotatedSourceToFailureCase branch May 22, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesHenry JamesHenry JamesHenry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rafaelss95 @JamesHenry