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

Scope improvements #168

Merged
merged 2 commits into from
Nov 27, 2018
Merged

Scope improvements #168

merged 2 commits into from
Nov 27, 2018

Conversation

FichteFoll
Copy link

Adding a custom embed_scope allows to exclude vue embeds from linting, for example, using source.js - source.js.embedded.vue, which would otherwise generate a lot of "xy not defined" errors.

For even more granularity, I added a meta.template.vue scope for mustache expressions.

Allows to exclude vue embeds from linting, for example, using
`source.js - source.js.embedded.vue`.
@skyronic
Copy link
Collaborator

Could you give me some steps to test and verify this change works?

@FichteFoll
Copy link
Author

FichteFoll commented Nov 13, 2018

I could provide a syntax test file that at least asserts my changes. Unless you are asking about the linter integration?

@skyronic
Copy link
Collaborator

I don't think we need an assertation test. Just would like a bit more information what kind of linting setup you tested it with, so I can reproduce the issue.

Some screenshots of before/after would also be good if possible. This is more to just document this for future purposes and changelogs, etc.

@FichteFoll
Copy link
Author

SublimeLinter-eslint with default settings:

2018-11-15_14-43-12

SublimeLinter-eslint with

            "selector": "source.js - source.js.embedded.vue"

and my PR:

2018-11-15_14-43-35

I was able to construct a selector that would exclude vue-embedded js code from linting, but it wasn't as simple. I don't have it anymore, but it was something along the lines of source.js - (text.html.vue source.js - source.js.embedded.html).

@skyronic
Copy link
Collaborator

Thank you for taking the time to put the information across. It's pretty clear why we need this change.

The code looks good and I'm going to merge it in after doing a couple of tests myself. Will this have any impact on users using the eslint plugin for vue?

@FichteFoll
Copy link
Author

No, it will only affect how Sublime Text scopes text and allows for better scope selection by linters. Currently, users still need to change their linter setting manually to profit from the change. I'm not sure if changing the default selector for SublimeLinter-eslint would be the right thing to do here, although I can hardly see a valid use case for linting these embeds when you have checks like unused-var enabled, which is the default iirc.

cc @braver

@skyronic skyronic merged commit 25ffe66 into vuejs:new Nov 27, 2018
@skyronic
Copy link
Collaborator

This looks ok. I'll continue testing with it for a few days, if I have any issues I might revert it, hope that's ok.

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

2 participants