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

Disable warnings through comments. #1861

Closed
wants to merge 4 commits into from

Conversation

RedHatter
Copy link
Contributor

Allows disabling warnings using comments. In this example

<div>
  <!-- warn-disable a11y-missing-attribute -->
  <a></a>
</div>

The svelte compiler would warn A11y: <a> element should have child content but not A11y: <a> element should have an href attribute.

The comments can be place anywhere on the line the error occurred or by themselves on the previous line. For a comment to be valid it must start with warn-disable followed by the error code to ignore. White-space is ignored. After the error code comments can contain any text (like a normal comment) to allow the developer to describe why the warning is being ignored.

The test whether of not to ignore a warning is done before custom onwarn or default_onwarn is called. This is to ensure cases like the rollup plugin don't override this functionality.

The name

I'm not sure warn-disable is the best name. Some alternatives

  • disable-warning
  • a11y-disable
  • a11y-ignore
  • warn-ignore
  • ignore-warning

leaving a11y out of the name might be a good idea to in case we ever add other types of warnings that might want to be ignored (unless we already do? I didn't see anything else). ignore-warning is more expressive of the purpose but warn-disable is closer to what eslint uses.

@maxmilton
Copy link
Contributor

maxmilton commented Nov 24, 2018

Good idea, especially if we continue to add more "linting" rules in future. Since the warning comes from svelte perhaps it should be something like <!-- svelte-disable a11y-missing-attribute -->?

If we go ahead with this, I'd like to also see *-disable, *-enable, *-disable-line, *-disable-next-line. But not necessarily as part of this PR.

Similar tools:

  • ESLint: /* eslint-disable rule-name */
  • TSLint: /* tslint:disable rule-name */
  • StyleLint: /* stylelint-disable rule-name */
  • markdownlint: <!-- markdownlint-disable MD001 -->
  • Istanbul: /* istanbul ignore next */

@RedHatter
Copy link
Contributor Author

You're right, I like svelte-disable.

-disable-line and -disable-next-line are easy enough. -disable and -enable are a bit more complex. I'll look into the changes soon.

@RedHatter
Copy link
Contributor Author

RedHatter commented Nov 25, 2018

-disable and -enable required handling the warnings compile side but the changes were minimal so I think it should be fine.

I like having svlete in the comment name but I'm not sure about simply svelte-disable. The linters that use that syntax are disabling their entire function (linting) for that section. Our comments disable one feature of svelte. Maybe something more verbose like svelte-disable-warning?

@maxmilton
Copy link
Contributor

I quite like svelte-disable actually. Easy to type and succinct. If we wanted something a little more verbose, how about svelte-disable-next?

Maybe anything more fancy than what you've created here could be done as future iterations. Or even keep as is, the simplicity of this feels right in line with svelte's simplicity :)

@stalkerg
Copy link
Contributor

For me main problem only with autofocus... I want to disable only this one warning.

@mikemaccana
Copy link

Is there any other way to disable the autofocus warning? I have a login form, removing autofocus to satisfy Svelte harms usability for most users.

@Conduitry
Copy link
Member

You can remove specific warnings by giving a custom onwarn function in your svelte config in your bundler. See e.g. https://github.com/rollup/rollup-plugin-svelte#usage

@mikemaccana
Copy link

Thanks @Conduitry that worked like a charm!

@Rich-Harris
Copy link
Member

Thanks for the PR @RedHatter. I think it's important to be able to specify which warnings are being disabled, and I'm nervous about the use of the code frame for this sort of thing (feels brittle), so I've opened a new PR, #3351. Will close this in favour of that

@Rich-Harris Rich-Harris closed this Aug 4, 2019
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

6 participants