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

feat(html-self-closing): add configuration presets #216

Merged
merged 29 commits into from
Aug 12, 2022
Merged

feat(html-self-closing): add configuration presets #216

merged 29 commits into from
Aug 12, 2022

Conversation

marekvospel
Copy link
Contributor

@marekvospel marekvospel commented Aug 11, 2022

see #190 (comment)

Changes

html-self-closing

added preset configuration option

'svelte/html-self-closing': ['warn', {
  'void': 'always',
  // ...
}]
// or
'svelte/html-self-closing': ['warn', 'all']

current presets:

  • none: all set to never
  • html: html-compliant (always for voit and svelte* elements, others never)
  • all: all on always

* to be discussed whether svelte elements should or shouldn't be self closing in html-compliant preset.

Progress

this PR is still a draft, html-self-closing tests are still using the old configuration, no preset tests have been added and the documentation for html-self-closing rule is also outdates

@marekvospel
Copy link
Contributor Author

*sorry for the amount of commits this PR is going to have. I'm in an environment where I can't install packages, so I can't use type checking, code formatting etc.

@marekvospel
Copy link
Contributor Author

@ota-meshi @JounQin could you please generate errors.json & output.svelte for the preset tests? I won't be able to do so until saturday / sunday. I've checked allow commits by maintainers, so you should have permissions to make changes in my fork

@JounQin
Copy link
Collaborator

JounQin commented Aug 11, 2022

I don't have time right now, I'd like to try when I'm free. 🍺

@marekvospel marekvospel marked this pull request as ready for review August 11, 2022 17:43
@marekvospel
Copy link
Contributor Author

The PR is ready! Once the invalid test outputs are generated and verified, it can be merged!

@changeset-bot
Copy link

changeset-bot bot commented Aug 12, 2022

🦋 Changeset detected

Latest commit: 88ea618

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-svelte Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@marekvospel marekvospel changed the title feat(html-self-closing)!: add configuration presets feat(html-self-closing): add configuration presets Aug 12, 2022
@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2022

This pull request introduces 1 alert when merging 49da137 into 931c062 - view on LGTM.com

new alerts:

  • 1 for Property access on null or undefined

@ota-meshi
Copy link
Member

We are about to introduce changeset and lgtm into this repository, but have not configured it yet. Ignore these messages for now.
#166

@marekvospel
Copy link
Contributor Author

I've updated the rule to take either a preset enum or config object, which makes this PR non-breaking

@JounQin
Copy link
Collaborator

JounQin commented Aug 12, 2022

I've updated the rule to take either a preset enum or config object, which makes this PR non-breaking

Right, so the PR description should be updated. 😁

@marekvospel
Copy link
Contributor Author

@ota-meshi @JounQin what do you think about the html preset? Should svelte special components like svelte:head be self closing?

src/rules/html-self-closing.ts Outdated Show resolved Hide resolved
@JounQin
Copy link
Collaborator

JounQin commented Aug 12, 2022

@ota-meshi @JounQin what do you think about the html preset? Should svelte special components like svelte:head be self closing?

According to Angular's HTML parser playground, <svelte:head /> is html compliant while <svelte-head /> is not, so yes.

src/rules/html-self-closing.ts Outdated Show resolved Hide resolved
@marekvospel
Copy link
Contributor Author

I've realised the test outputs the errors, so I've copied it into the test output files, so the tests should pass now

@marekvospel
Copy link
Contributor Author

so many typos lmao 😂

Copy link
Collaborator

@JounQin JounQin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts!

@JounQin JounQin requested a review from ota-meshi August 12, 2022 23:28
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

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

3 participants