Check for remote files #1017

Arcanemagus · 2017-09-17T05:37:09Z

Add several sanity checks before moving on to attempting to lint the file:

Check that the TextEditor is valid
Check that the path for the file is not empty
Check that the path isn't a remote path

If a remote path is found, show a message to the user that linting isn't possible.

Blocked on #1015 as it uses the Error handling functionality introduced in that branch. Just putting this up here so it can get reviewed.

Fixes #594.

skylize · 2017-09-17T17:37:06Z

Should possibility of remote file checking be on our radar?

Arcanemagus · 2017-09-17T19:06:50Z

For simple linters that just depend on the file contents it's certainly a possibility, but eslint depends on a large amount of state in the rest of the project surrounding the file being linted. This means we need full access to the file system surrounding the file when linting it which isn't possible with a remote file.

skylize · 2017-09-18T04:59:35Z

For simple linters that just depend on the file contents it's certainly a possibility, but eslint depends on a large amount of state in the rest of the project surrounding the file being linted.

I think treating remote files as a single file with no project context would be a reasonable trade-off. If we get an .eslintrc global fallback working, it seems quite reasonable to use that fallback on new files that have never been saved. We could then simply treat remote files as new/never-saved.. Worth thinking about at least I think.

Arcanemagus · 2017-09-18T06:28:52Z

I think treating remote files as a single file with no project context would be a reasonable trade-off.

The most that could be done is ESLint's syntax check, and even then I'm not sure if reporting fixes back would even work as I don't know if the messages can be associated back to the file in any meaningful way.

We could then simply treat remote files as new/never-saved.

Linter shouldn't even be triggering us on unsaved files in the first place 😉.

Add several sanity checks before moving on to attempting to lint the file: * Check that the `TextEditor` is valid * Check that the path for the file is not empty * Check that the path isn't a remote path If a remote path is found, show a message to the user that linting isn't possible. Fixes #594.

skylize · 2017-09-18T16:16:32Z

The most that could be done is ESLint's syntax check, and even then I'm not sure if reporting fixes back would even work as I don't know if the messages can be associated back to the file in any meaningful way.

A basic syntax check is still a very useful feature of having a linter handy. If I'm editing a remote or unsaved file and I have choice between linter only does basic syntax check or linter ignores the file, I would definitely take the syntax check. As for whether we can report the findings, that's I don't know about, but if there is a reasonable way it would be a good feature.

We could then simply treat remote files as new/never-saved.

Linter shouldn't even be triggering us on unsaved files in the first place 😉.

Yes. This statement was based on the premise of first having useful treatment for unsaved files, which we currently do not.

I just think it's worth having this potential use-case in the back of our minds in case someone stumbles onto a good way to do it or wakes up from a dream about linting with the perfect solution. Hahaha

Arcanemagus · 2017-09-18T16:26:10Z

Editing a remote or unsaved file

There's a slim possibility a remote file would work, but as there is no path at all for an unsaved file there is no way to report messages to it (This is why linter doesn't support unsaved files currently, the API would have to be modified to pass a reference to an associated TextEditor on every message).

Someone stumbles onto a good way to do it or wakes up from a dream about linting with the perfect solution

It's been over a year with no better solutions presented in that issue, so I went with the safest one 😉.

IanVS

Personally, I think disabling for remote files is fine. If we try to support them, I think we're in for a bad time for the reasons @Arcanemagus has mentioned. It also doesn't sound like there's anyone clamoring for us to support that use case, so I think our time is better spent elsewhere.

I do waffle a bit about whether this should be an error or a warning. It's not like there's definitely anything wrong with the file, we just don't know, so an error seems a bit… harsh. What do you think about making a helpers.handleWarning?

IanVS · 2017-09-19T01:30:00Z

src/main.js

+        if (filePath.includes('://')) {
+          // If the path is a URL (Nuclide remote file) return a message
+          // telling the user we are unable to work on remote files.
+          const message = 'Remote file open, impossible to run ESLint.'


Perhaps a slightly-more-accurate message might be something like:

"Remote file open, linter-eslint is therefore disabled for this file."

Since it sounds like the Facebook folks have may their own eslint integration for remote files.

Huh, it would have to be some sort of primitive LSP to work at all. They did start Nuclide before Microsoft published the LSP protocol.

IanVS · 2017-09-19T01:30:27Z

src/main.js

@@ -188,11 +188,26 @@ module.exports = {
      scope: 'file',
      lintsOnChange: true,
      lint: async (textEditor) => {
-        const text = textEditor.getText()
-        if (text.length === 0) {
-          return []


Question

Did you mean to remove this check?

Hmmm, nope. I meant to move it down below the path checks along with the getText call. 🐛

Oh, actually, yes I did mean to remove it. With it in there we are saying that an empty file is completely valid, but we have no idea what a user's configuration is, or what plugins they have or rules they have enabled. We have no way of knowing whether an empty file is considered "valid" under their configuration so we should remove this optimization and do the full ESLint run even on an empty file.

Add a function to generate generic messages to send to the user, modify `handleError` to use this function and change the message dispalyed for remote files to a warning.

Arcanemagus · 2017-09-19T03:50:04Z

@IanVS fixed it up a bit 😉.

IanVS

I pushed up a small commit to expand the jsdoc for generateUserMessage, but this all looks great.

tim-soft · 2018-02-28T19:04:17Z

Just for clarity, there's definitely no way we can lint projects inside atom in remote projects via Nuclide + Watchman?

Arcanemagus · 2018-02-28T19:06:40Z

All that we have for files opened like that is the raw text, since ESLint depends on external configuration files which we have no way to access with these remote files there really isn't anything that could be done other than possibly a basic syntax check.

Just because you can see the text in Atom doesn't mean the hacked together way that Nuclide has done that works for anything other than Nuclide.

tim-soft · 2018-02-28T19:11:04Z

Even if there were some ssh tunnel to the remote project dir?

Arcanemagus · 2018-02-28T19:12:37Z

You could use sshfs or something similar and open the directory "locally" so it is accessible as part of the filesystem, the remote URL files aren't usable though.

tim-soft · 2018-02-28T19:15:53Z

I guess running some kind of eslint server to report back linting results to atom from the remote project would be inadvisable as well? Linting on remote projects is a real pain, but I understand the limitation

Arcanemagus · 2018-02-28T19:18:25Z

An ESLint Language Server is certainly possible, but none exists currently that I know of. From what I understand there is nothing that prevents a language server from running on a remote host, I don't think any of the current adapters into Atom work like that though.

tim-soft · 2018-03-01T01:26:17Z

Ah, I see. Making an eslint-js language client seems like the only way something like this would integrate with atom super nicely. https://github.com/atom/atom-languageclient

Thanks for the clarification

Arcanemagus added bug DO NOT MERGE labels Sep 17, 2017

Arcanemagus self-assigned this Sep 17, 2017

Arcanemagus requested a review from IanVS September 17, 2017 05:37

Arcanemagus changed the base branch from master to arcanemagus/better-error-handling September 17, 2017 05:37

Arcanemagus force-pushed the arcanemagus/ignore-indirect-paths branch from 8854efe to df2841e Compare September 18, 2017 01:48

Arcanemagus changed the base branch from arcanemagus/better-error-handling to master September 18, 2017 01:48

Arcanemagus removed the DO NOT MERGE label Sep 18, 2017

Arcanemagus force-pushed the arcanemagus/ignore-indirect-paths branch from df2841e to 38d13cb Compare September 18, 2017 03:38

Arcanemagus force-pushed the arcanemagus/ignore-indirect-paths branch from 38d13cb to fa3079d Compare September 18, 2017 07:13

IanVS reviewed Sep 19, 2017

View reviewed changes

Add generateUserMessage

5b87092

Add a function to generate generic messages to send to the user, modify `handleError` to use this function and change the message dispalyed for remote files to a warning.

Add options to jsdoc

aa3e558

IanVS approved these changes Sep 27, 2017

View reviewed changes

Arcanemagus merged commit 4254301 into master Sep 27, 2017

Arcanemagus deleted the arcanemagus/ignore-indirect-paths branch September 27, 2017 16:31

Arcanemagus mentioned this pull request Oct 18, 2017

"Path must be a string. Received undefined" in untitled JavaScript tab #1037

Closed

tim-soft mentioned this pull request Mar 2, 2018

Support Linters like ESlint on Remote Projects facebookarchive/nuclide#1475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for remote files #1017

Check for remote files #1017

Arcanemagus commented Sep 17, 2017

skylize commented Sep 17, 2017

Arcanemagus commented Sep 17, 2017

skylize commented Sep 18, 2017

Arcanemagus commented Sep 18, 2017

skylize commented Sep 18, 2017

Arcanemagus commented Sep 18, 2017

IanVS left a comment

IanVS Sep 19, 2017

Arcanemagus Sep 19, 2017

IanVS Sep 19, 2017

Arcanemagus Sep 19, 2017

Arcanemagus Sep 19, 2017

Arcanemagus commented Sep 19, 2017

IanVS left a comment

tim-soft commented Feb 28, 2018 •

edited

Loading

Arcanemagus commented Feb 28, 2018 •

edited

Loading

tim-soft commented Feb 28, 2018

Arcanemagus commented Feb 28, 2018

tim-soft commented Feb 28, 2018 •

edited

Loading

Arcanemagus commented Feb 28, 2018

tim-soft commented Mar 1, 2018

Check for remote files #1017

Check for remote files #1017

Conversation

Arcanemagus commented Sep 17, 2017

skylize commented Sep 17, 2017

Arcanemagus commented Sep 17, 2017

skylize commented Sep 18, 2017

Arcanemagus commented Sep 18, 2017

skylize commented Sep 18, 2017

Arcanemagus commented Sep 18, 2017

IanVS left a comment

Choose a reason for hiding this comment

IanVS Sep 19, 2017

Choose a reason for hiding this comment

Arcanemagus Sep 19, 2017

Choose a reason for hiding this comment

IanVS Sep 19, 2017

Choose a reason for hiding this comment

Arcanemagus Sep 19, 2017

Choose a reason for hiding this comment

Arcanemagus Sep 19, 2017

Choose a reason for hiding this comment

Arcanemagus commented Sep 19, 2017

IanVS left a comment

Choose a reason for hiding this comment

tim-soft commented Feb 28, 2018 • edited Loading

Arcanemagus commented Feb 28, 2018 • edited Loading

tim-soft commented Feb 28, 2018

Arcanemagus commented Feb 28, 2018

tim-soft commented Feb 28, 2018 • edited Loading

Arcanemagus commented Feb 28, 2018

tim-soft commented Mar 1, 2018

tim-soft commented Feb 28, 2018 •

edited

Loading

Arcanemagus commented Feb 28, 2018 •

edited

Loading

tim-soft commented Feb 28, 2018 •

edited

Loading