Hiding 'ESLint: Fix file' in non-supported files #868
Conversation
| // Marks each JavaScript editor with a CSS class so that | ||
| // we can enable commands only for JavaScript editors. | ||
| this.subscriptions.add(atom.workspace.observeTextEditors((textEditor) => { | ||
| if (textEditor.getRootScopeDescriptor().scopes |
There was a problem hiding this comment.
Can you change this to match the current checks, in fact that check should just be moved to a helper function and called in both places.
There was a problem hiding this comment.
The downside to the check you linked to is that it's only for the scope of where the current cursor is. Where as my code is checking the scope of the file itself.
There was a problem hiding this comment.
That's actually the upside, not downside. Say for example you are editing an HTML file with some embedded JS code. The scope of the file would be source.html, while the scope of the embedded JS will be source.js.embedded.html. If we just went with your current method the file would never enable the command for the embedded JS code, while checking the cursor means that if the user has a cursor in a scope that the linter provider understands it will be enabled.
This is how linter itself triggers providers in the first place, and one of the reasons I'm not exactly sold on the whole idea of adding a class to the editor instead of just checking things when called since we aren't really sure what situations the user can get into.
For other linters where there is a nice pre-defined list of possible scopes it might work, but for here I'm not sure I see how it will work without a ton of hacky workarounds (observing all editor cursors and checking if one ever enters a supported scope in the editor...).
There was a problem hiding this comment.
That makes sense and I agree. The issue I'm trying to address is when you have many linters installed, and you open the command pallet, start typing "fix" and then the fix commands for all your linters appear. Now you have the manual task of finding the right one.
The only way I know of how to scope commands is via CSS selectors (at least according to the atom API). With that in mind I see 2 options, either:
- Drop this PR and allow the command in all scopes
- Monitor each time the cursor moves, check the scope and add/remove the CSS selector
- I don't like this option since it sounds pretty expensive and could slow the editor and ruin UX.
What are your feelings on this? Depending on your answser I will also amend my TSLint PR accordingly.
There was a problem hiding this comment.
Hmmmm, I think I would say that it will work for providers with well defined allowed scope lists (like TSLint), and in fact for those the getRootScopeDescriptor might even be a better choice.
For providers like this one though where the allowed scopes can change at any time, this doesn't really make much sense, and to make it work at all would involve watching quite a bit of the components of Atom to ensure you get everything, so unfortunately I think for linter-eslint at least this doesn't really make much sense.
There was a problem hiding this comment.
Okay, I shall abandon this PR and leave my TSLint PR as is. Thanks for your input =]
There was a problem hiding this comment.
Thanks for sticking through with it while I figured out what felt "wrong" about it 😛.
I'll do a re-review of the TSLint one in a bit and most likely merge it.
| @@ -93,7 +94,16 @@ module.exports = { | |||
| } | |||
| })) | |||
|
|
|||
| this.subscriptions.add(atom.commands.add('atom-text-editor', { | |||
| // Marks each JavaScript editor with a CSS class so that | |||
| // we can enable commands only for JavaScript editors. | |||
There was a problem hiding this comment.
This linter supports any file type the user desires since there are ESLint processors for some pretty strange templating languages out there. JavaScript should be changed to supported.
|
Closing based on the above discussion. |
No description provided.