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

[eslint-config-xo] - Allow v8 alongside c8 in capitalized-comments #765

Closed
Primajin opened this issue Aug 4, 2024 · 2 comments · Fixed by xojs/eslint-config-xo#88
Closed

Comments

@Primajin
Copy link
Contributor

Primajin commented Aug 4, 2024

Hey there, I would like to allow v8 alongside the already existing c8 (xojs/eslint-config-xo@dea9534 / xojs/eslint-config-xo#65) - so that it can be used in ignore comments for @vitest/coverage-v8.

https://github.com/xojs/eslint-config-xo/blob/a09d65618cc83ac27eba9dc5b85423c26807e9ac/index.js#L305

- ignorePattern: /pragma|ignore|prettier-ignore|webpack\w+:|c8|type-coverage:/.source,
+ ignorePattern: /pragma|ignore|prettier-ignore|webpack\w+:|c8|v8|type-coverage:/.source,

Are you ok with this change, then I'm happy to whip up a PR.

Thanks in advance
Best

@sindresorhus
Copy link
Member

PR welcome. It should be the whole string though, not just v8.

Note that it won't be available in XO until #702 is done.

@Primajin
Copy link
Contributor Author

Primajin commented Aug 5, 2024

Hey there @sindresorhus , thanks for the info - however I didn't fully understand what you meant by "the whole string". I went ahead on opening the PR, hoping that we can refine together what was meant.

The comments to skip lines from being gathered into coverage by v8 will look something like this:

/* v8 ignore next 3 */

For example to ignore the "next 3 lines". There are also other key words in order to just skip the else or similar thing, but I didn't find a good comprehensive list of what is possible.

I have tested it in my other repos and indeed v8 works but V8 will not. So v8 has to be ignored if it stands at the beginning of a comment string.

Furthermore I wanted to check on testing - I ran npm test and everything went fine, however I am not sure I understand what it is actually testing. I was expecting test cases that expect and assert strings before and after they went through the XO config and check if the intended outcome was performed by the rules set. --> If this is wanted, I am happy to add tests.

Lastly to your point on waiting for flat config, that is totally fine with me, I'm really looking forward to the change and won't get in the way 😄

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 a pull request may close this issue.

2 participants