-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: warn on unknown warning codes in runes mode #11549
Conversation
|
packages/svelte/tests/validator/samples/static-state-reference/input.svelte
Outdated
Show resolved
Hide resolved
I personally prefer the other approach of telling the people inline "hey this did work in legacy mode but now it no longer does". Now when converting to runes they would get two warnings and you have to connect them manually in your head (to know that they "belong" together). |
I could be persuaded to introduce the "unknown selector warning" if
|
We absolutely need the I hadn't considered the 'prose as part of the ignore comment' situation. I think we can solve that unambiguously with a small change — make the codes comma-separated rather than space-separated, and anything after the comma-separated list gets ignored: <!-- svelte-ignore this_warning, that_warning because they're handled elsewhere --> If we did that we'd want to tell people who did Adding the context to the no-longer-silenced warning feels like a mistake to me. For one thing, the |
I think context is crucial for the migration story because it works in legacy mode and then you opt into runes for the component and suddenly there's a warning - and you're like "huh, what do you mean that code is unrecognized now?". Giving the hint that it was a backwards compat feat clues you into why this worked before and gives you a "ah thanks for thinking of that" feeling. As for the heuristic: I would say commas are not necessary, instead treat every word that has a dash or underscore as a potential warning, and warn for everything that's not matching that's before the first non-code-looking word. |
Adjusted the warning message to make it clear that this is a 4 -> 5 change — there's now The prose thing is still a TODO. I'd prefer the comma-separated list because it's ambiguous otherwise — is <!-- svelte-ignore this_thing that_thing is not this_thing --> ESLint uses comma-separated lists, so it's not like we're going rogue. |
Ok let's go with commas then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'll do migration and a proposal for a simpler ignore traversal in a separate PR)
As for „prose as part of the ignore comment” syntax — PHPStan just introduced error identifiers, and they use parentheses for that, which looks very natural and removes any ambiguity:
(from https://phpstan.org/blog/phpstan-1-11-errors-identifiers-phpstan-pro-reboot#error-identifiers) |
Interesting. I think that's probably a reasonable constraint — further eliminates ambiguity, and encourages ecosystem-wide consistency. Thoughts @dummdidumm? |
I'm not sure if the braces bring us any additional value if we require all the error codes separated by commas upfront. We could add it alter if you want to add the reason right after the code |
Alternative to #11495. WIP
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint