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

fix(eslint-plugin-template): fix fixer of inline templates #1472

Merged
merged 1 commit into from Sep 16, 2023

Conversation

json-derulo
Copy link
Contributor

@json-derulo json-derulo commented Aug 10, 2023

Currently, the rule fixer does not return correct results for inline templates. It applies to all template rules with a fixer, see #1464 (comment). This is fixed by this PR.

Closes #1464

Note: I wanted to add some tests, but I'm not sure where to start. I performed manual tests of all the cases described in the linked issue.

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.

Thanks @json-derulo!

When you say you weren't sure where to start... Did you consider adding the cases you reported to the relevant rule's test cases?

You just need to look at the existing tests, duplicate one or more and amend.

We will definitely want to capture your findings as tests before proceeding

@json-derulo
Copy link
Contributor Author

@JamesHenry added a test case

@JamesHenry
Copy link
Member

@json-derulo I meant more capturing the real experience you were having using rules e.g. here #1464 (comment)

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1472 (ccf6f85) into main (2e4af4d) will increase coverage by 0.16%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1472      +/-   ##
==========================================
+ Coverage   89.23%   89.39%   +0.16%     
==========================================
  Files         162      162              
  Lines        3056     3056              
  Branches      521      521              
==========================================
+ Hits         2727     2732       +5     
+ Misses        201      195       -6     
- Partials      128      129       +1     
Flag Coverage Δ
unittest 89.39% <ø> (+0.16%) ⬆️

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

Files Changed Coverage Δ
packages/eslint-plugin-template/src/processors.ts 76.74% <ø> (+5.81%) ⬆️

@json-derulo
Copy link
Contributor Author

@JamesHenry I am a bit stuck trying to add tests for the real cases. I have tried to setup test cases with the RuleTester. When I use parser @typescript-eslint/parser, I'm always getting the following error:

Error while loading rule: You have used a rule which requires '@angular-eslint/template-parser' to be used as the 'parser' in your ESLint config.

I have tried to extend from @angular-eslint/template/process-inline-templates, use processor @angular-eslint/template/extract-inline-html, plugin @angular-eslint/template and several combinations of those. The error persists.

When I use parser @angular-eslint/template-parser no errors are reported for the invalid cases, it seems like it cannot understand the TypeScript syntax.

Do you have any other idea how I could test this?

@JamesHenry
Copy link
Member

Yes sorry I think I was skipping some relevant complexity here, it probably isn’t a good fit for the unit tests, it would be better as an integration test.

I’m on my phone and thinking out loud but I think we could add a relevant file to the generated fixture in one of the existing integration tests and snapshot it before and after applying fixing.

If you have time to play around with this idea in the next few days then that’s great, otherwise I can take a proper look this weekend sometime

@json-derulo
Copy link
Contributor Author

@JamesHenry thanks for your input, I think I figured it out by myself. I've implemented an integration test which generates a fresh Angular Workspace and runs Angular ESLint Add schematics. Afterwards, a new test component file is generated with the test cases. I excluded the prefer-self-closing-tags rule there because it's not part of the recommended rule set and would require additional steps. Finally, the lint fixer is executed, and the resulting file content is checked against a snapshot.

Not sure why the CI is failing, on my machine the integration tests succeed. Also the failure is related to a different integration test which was not changed.

@nx-cloud
Copy link

nx-cloud bot commented Aug 25, 2023

☁️ Nx Cloud Report

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

📂 See all runs for this branch


✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

@json-derulo
Copy link
Contributor Author

@JamesHenry after rebasing the latest changes from the main branch, CI is green now. Could you have a look again to the changes of this PR?

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.

Great work, thank you @json-derulo!

@JamesHenry JamesHenry merged commit 470e12b into angular-eslint:main Sep 16, 2023
14 checks passed
strawberry-choco pushed a commit to strawberry-choco/angular-eslint that referenced this pull request Sep 17, 2023
@json-derulo json-derulo deleted the inline-template-fixer branch September 18, 2023 15:12
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.

[prefer-self-closing-tags] auto-fix results in invalid HTML
2 participants