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): [prefer-self-closing-tags] always ignore index.html files #1865

Merged

Conversation

Squixx
Copy link
Contributor

@Squixx Squixx commented Jun 6, 2024

Ignores index.html files for prefer-self-closing-tags as described in #1857

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.

You should be able to add a unit test for this by setting filename please

@Squixx
Copy link
Contributor Author

Squixx commented Jun 17, 2024

added!

Copy link

nx-cloud bot commented Jun 17, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9ec2155. 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.

@JamesHenry JamesHenry changed the title feat: ignore index.html for self-closing-tags fix(eslint-plugin-template): ignore index.html for self-closing-tags Jun 17, 2024
@JamesHenry JamesHenry changed the title fix(eslint-plugin-template): ignore index.html for self-closing-tags fix(eslint-plugin-template): [prefer-self-closing-tags] always ignore index.html files Jun 17, 2024
@JamesHenry
Copy link
Member

Test looks good, please run yarn update-rule-docs and commit the result, the docs are generated from the unit tests so whenever they change the docs need to too

@Squixx
Copy link
Contributor Author

Squixx commented Jun 17, 2024

i've updated the docs. However currently the result doesn't make sense in this context. Is there a ways to simply annotate a rule output? e.g. src/index.html is excluded as this would prevent angular from bootstrapping.

I could add a simple comment input to the markdown converter, but not sure what would be wanted here.

@JamesHenry
Copy link
Member

Ah yeah good spot, let's please extend the rules doc generation logic to print the filename above the code snippet in the case that it is explicitly set in the unit test, e.g. the result would look like the below:


Filename: src/index.html

<app-root></app-root>

Copy link

codecov bot commented Jun 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.63%. Comparing base (2f9e378) to head (9ec2155).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1865   +/-   ##
=======================================
  Coverage   91.62%   91.63%           
=======================================
  Files         179      179           
  Lines        3380     3382    +2     
  Branches      547      548    +1     
=======================================
+ Hits         3097     3099    +2     
  Misses        144      144           
  Partials      139      139           
Flag Coverage Δ
unittest 91.63% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
...gin-template/src/rules/prefer-self-closing-tags.ts 97.43% <100.00%> (+0.13%) ⬆️
...late/tests/rules/prefer-self-closing-tags/cases.ts 100.00% <ø> (ø)

@JamesHenry JamesHenry merged commit acdc4a6 into angular-eslint:main Jun 17, 2024
7 checks passed
@JamesHenry
Copy link
Member

Thanks so much @Squixx!

@Squixx
Copy link
Contributor Author

Squixx commented Jun 17, 2024

Thanks for guiding me through the repo.

@silvanadrian
Copy link

@JamesHenry Is there a plan on getting this fix released anytime soon?

@JamesHenry
Copy link
Member

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

4 participants