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

Automatically detect eslint-plugin-html, and change default selector settings when found #253

Closed
Standard8 opened this issue May 24, 2018 · 10 comments

Comments

@Standard8
Copy link

@Standard8 Standard8 commented May 24, 2018

I'm using SublimeText 3.1.1, SublimeLinter 4.6.0, SublimeLinter-eslint 4.1.3.

We first discussed this in #87. I have a multi-developer repository, where we have command line operation, and different editors in use. We really need Sublime & ESLint to work together, so that we can lint html files correctly in all editors and on the command line.

I've discovered that I can get both to work by implementing a user setting of:

{
  "linters": {
    "eslint": {
      "selector": "source.js, text.html.basic, text.xml",
    }
  }
}

Rather than try to get multiple developers to set this up, I'd like a way where SublimeLinter-eslint could detect and set this automatically.

I've looked into Sublime's project settings files, but they seem limited - for example, we don't have one at the moment, so existing developers wouldn't be able to be updated automatically (unlike VSCode's default config options).

@kaste
Copy link
Contributor

@kaste kaste commented May 25, 2018

Hi! This is an interesting feature but currently not achievable in 5 lines code. If you have python people in your team, I can guide them. We need two refactorings in core SL and then the feat implementation itself.

@kaste
Copy link
Contributor

@kaste kaste commented May 28, 2018

I already did one of the two refactorings. 👍

@Standard8
Copy link
Author

@Standard8 Standard8 commented Jun 13, 2018

@kaste if you can describe what needs doing, I can try and find someone (or I might be able to do it myself).

@Standard8
Copy link
Author

@Standard8 Standard8 commented Aug 28, 2018

@kaste, did you see my comment above?

@kaste
Copy link
Contributor

@kaste kaste commented Aug 28, 2018

Oops. I'll try to write it down this week. Sorry.

@kaste
Copy link
Contributor

@kaste kaste commented Aug 29, 2018

Okay, how can we tackle this one?

Instead of working with file extensions, we can identify the applied syntax of a buffer via selectors or scopes. A typical js file has the main scope source.js, html files have the base scope text.html.basic.

We of course need a mapping from selectors to eslint-plugins. Or vice-versa. This depends on if we imagine that for one syntax/selector there are actually multiple plugins available, or again vice-versa one plugin supports multiple syntaxes. So maybe, we have a mapping from

  'source.vue': 'eslint-plugin-vue',
  'text.html.basic': 'eslint-plugin-html'

The first method SL will interact with is

    @classmethod
    def can_lint_view(cls, view, settings):

which gets a view/buffer with the computed settings and returns a boolean.

    (sublime.View, Dict) -> bool

We return True/False and thereby decide if eslint should lint this view. The settings object holds the default selector (E.g. settings.get('selector')), but this value can be mutated, because it is only used here and now and for this thread.

So, we could, given a view, extend the selector.

If we have a view, we call view.scope_name(0) -> str to get the scope at the first char. The scope is a space seperated list of selectors, the main one is at the left. So we could do base_selector = scope.split(' ')[0].

Now, we translate this base_selector via our map above and get None or a plugin name back. This plugin is required to process that buffer, so we lookup the nearest package.json and see if the plugin is a dependency (or devDependency etc.).

Given a view, we get its context via SublimeLinter.lint.linter.get_view_context(view) -> Dict. We ask context.get('file_path') for a potential path to the file we're editing. Or context.get('folder') for the base directory associated with the current window. (Note: The edge cases here are, that the user is editing an unsaved/unnamed buffer in a just opened, new window. So, the code must prepare for Nones here.)

In case we have a starting path, we thus search for package.json going upwards until we find the nearest. If we find one, we just read and evaluate it.

Finally, we mutate the selector, maybe like so

    settings['selector'] = "{},{}".format(base_selector, settings.get('selector', '') 

and return calling super().can_lint_view(...).

(Implementations wise, we prefer functions over methods. Some of these functions like get_package_json_content(view) -> Dict or search_for_file('package.json', start_dir) can be pulled into main SL.)

@TehShrike
Copy link

@TehShrike TehShrike commented Feb 4, 2019

I would like to be able to pass full html files to eslint, but I do not want this plugin to try to detect that automatically.

I would prefer to be able to set an option in SublimeLinter-eslint that would tell it whether to use the js selector, or the html selector.

@kaste
Copy link
Contributor

@kaste kaste commented Feb 9, 2019

@TehShrike You can just set the html selector, the js one, or both manually. What should the option do?

@TehShrike
Copy link

@TehShrike TehShrike commented Feb 9, 2019

Unset this value:

'selector': 'source.js - meta.attribute-with-value'

kaste added a commit that referenced this issue Nov 10, 2019
Fixes #253
Closes #284

The strategy here is to use a wide `selector` in the beginning and
then narrow it down during actual linting.

SublimeLinter needs a good `selector` as a **fast** check to match
views and installed linters because this first check runs on a shared
worker. T.i. we cannot just select *all* views and then decide later.

After the first check, it is always possible to abort loud or silently.
We do here silently because it's not a configuration error coming
from the user but an automatic algorithm bailing out.

With this implementation there comes the drawback that we have to flip
the previous default behavior. The default behavior was for the linter
to run on parts of the buffer marked as `source.js` concurrently. E.g.
it did run on the `<script>` tags when viewing a HTML file.

However, this default often did not produce good results. I.e. in an
HTML file it has no notion of gloabls introduced in one script which
are accessible in another script.

If the user wants this behavior, she now has to opt in by setting the
old selector `'source.js - meta.attribute-with-value'` in the settings
on her own.

Implementation risks:

- We deep import `read_json_file`. It is possible that this import can
  break since deep imports are not protected by any deprecation policy.

- We have two side-effects: `self.notify_unassign()` followed by
  `raise PermanentError()`. It is possible that in the future we need
  to signal the same outcome differently.
@kaste kaste mentioned this issue Nov 10, 2019
2 of 2 tasks complete
@kaste
Copy link
Contributor

@kaste kaste commented Nov 10, 2019

Better now then never. PR is at #287. Would be nice if someone you could test this.

kaste added a commit that referenced this issue Nov 20, 2019
Fixes #253
Closes #284

The strategy here is to use a wide `selector` in the beginning and
then narrow it down during actual linting.

SublimeLinter needs a good `selector` as a **fast** check to match
views and installed linters because this first check runs on a shared
worker. T.i. we cannot just select *all* views and then decide later.

After the first check, it is always possible to abort loud or silently.
We do here silently because it's not a configuration error coming
from the user but an automatic algorithm bailing out.

With this implementation there comes the drawback that we have to flip
the previous default behavior. The default behavior was for the linter
to run on parts of the buffer marked as `source.js` concurrently. E.g.
it did run on the `<script>` tags when viewing a HTML file.

However, this default often did not produce good results. I.e. in an
HTML file it has no notion of gloabls introduced in one script which
are accessible in another script.

If the user wants this behavior, she now has to opt in by setting the
old selector `'source.js - meta.attribute-with-value'` in the settings
on her own.

Implementation risks:

- We deep import `read_json_file`. It is possible that this import can
  break since deep imports are not protected by any deprecation policy.

- We have two side-effects: `self.notify_unassign()` followed by
  `raise PermanentError()`. It is possible that in the future we need
  to signal the same outcome differently.
@kaste kaste closed this in #287 Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants