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

allow linting of new yet unsaved files #1235

Open
dirk-thomas opened this issue Sep 7, 2016 · 10 comments
Open

allow linting of new yet unsaved files #1235

dirk-thomas opened this issue Sep 7, 2016 · 10 comments

Comments

@dirk-thomas
Copy link

Created as a follow up of AtomLinter/linter-xmllint#68.

Currently when a user creates a new file (but doesn't save it yet) and selects a specific syntax for the editor no linter is being triggered. Some linters might not be able to work with an editor which doesn't have a path yet e.g. because they don't support on-the-fly linting or require relative resources. But some linters can perform their task in that case and they should be triggered.

This will require linters which can't handle this case to check the editors path and abort linting when the editor doesn't have a path yet.

@steelbrain steelbrain added this to the v2.0.0 milestone Sep 7, 2016
@dirk-thomas dirk-thomas changed the title allow linting on new yet unsaved files allow linting of new yet unsaved files Sep 7, 2016
@gliviu
Copy link

gliviu commented Sep 7, 2016

Just a thought, if this change would break many linters why not allow the linter to pass this information (whether it supports on-the-fly or not) to the base linter.
This would be false by default so that base linter always do the 'file saved' check if parameter not provided.

@Arcanemagus
Copy link
Collaborator

@gliviu That's already how the linter providers work 😉. There is a lintOnFly (soon to be lintsOnChange) flag they set and send to Linter when they are initialized. The only consideration here with regards to that is the edge case of potentially requiring external resources... and how the current file path is determined.

All of the linters that I know of currently either use path.dirname(textEditor.getPath()) (which will return undefined), or atom.project.relativizePath(textEditor.getPath())[0]... with a fallback to the previous. The problem is that this will return null or undefined, which I'm not sure any of them currently handle that situation well.

I'd be fine with this being a required situation to be able to handle in v2, as technically any v1 linter should already be able to handle this, it's just none were written well enough to do so that I know of 😛.

@Arcanemagus
Copy link
Collaborator

As for actually implementing this I see a few options for linters that need a path to operate (for things like determining configurations):

  • Just return [] until the file is actually saved and has a path
  • Fall back to atom.project.getPaths()[0], this has the slight potential of being wrong though if the user has multiple projects open in the same Atom window. From what I've seen though this is a pretty rare situation.
  • Use a default configuration

For linters that don't need a path to operate (ie. they have no configuration) this isn't really an issue.

I'd personally probably go with the second choice, since in the rare case that there are more than one projects open in the same window, I'd imagine they are related and would probably have the same configuration anyway. If not, then the configuration will be wrong and there might be some incorrect messages reported, at least until the file is saved.

The technically correct result would be to just return [] if a path is necessary to operate though.

What are people's thoughts on that?

@dirk-thomas
Copy link
Author

dirk-thomas commented Sep 7, 2016

Some of the current on-the-fly linters already don't care about the getPath() but only about the getText() of the passed editor. Just some examples I am aware of:

I can see atom.project.getPaths()[0] being wrong in various cases. E.g. the linter uses relative resources (e.g. a dtd to validate a xml file). Many linters consider configuration files relative to (or in parent folders of) the processed file. Often the editor path is also used in the error message. I guess each linter has to decide what makes sense.

A linter could also decide to return one message mentioning that the linter could not be run successful and that the file needs to be saved before.

@Arcanemagus
Copy link
Collaborator

Arcanemagus commented Sep 7, 2016

linter-pyflakes does use the file path, just not in the call to pyflakes. It's used directly as the filePath in the message (which will be required in v2 linters). Same thing with linter-pep8.

And that brings up an excellent problem with this... the v2 Message type currently requires a file in the location, this can never be available in this situation 😕.

@steelbrain
Copy link
Owner

@Arcanemagus that is actually a very good point. If we allow this, we'll have to make providers associate textEditors with messages, because we not only have file scoped providers, we also have indie and project ones

@steelbrain
Copy link
Owner

If someone needs a few reasons on why textEditors with messages are bad, try adding a text editor about a file that isn't open yet 😉

@Arcanemagus
Copy link
Collaborator

Arcanemagus commented Sep 7, 2016

Ouch, so we would have to have an active TextEditor associated with each message? I'm not sure that's something we would want to do...

@pablomayobre
Copy link

pablomayobre commented May 11, 2017

How about generating an "unsaved-file-path" for unsaved files, which get passed to the linter-provider? when a message file path matches the "unsaved-file-path" associate the message with said Buffer.

So all this stuff discussed here would actually be done mostly in the Linter side and not in the providers, old providers would ignore this information so it wouldn't be a breaking change, everything else stays the same

robwise added a commit to prettier/prettier-atom that referenced this issue Aug 27, 2017
Previously we were simply exiting early during the error handling process if there was no filepath
because the linter package requires a filepath in order to work. Now, we detect this early and
instead send the error message to a popup so that the user still gets some feedback.

See: steelbrain/linter#1235

Fixes #235
@jinglesthula
Copy link

I'm late to the party, but I'd like to toss out the one case where I'd find this hugely useful. I often copy/paste JSON network responses into a new Atom file to format them for readability. I'll occasionally miss a leading/trailing character, or the JSON will be malformed to start with for some reason. If the linter could be run on this throw-away context to help me make the JSON valid so the pretty formatter can operate on it.

Other similar quick linting on copy/pasted chunks of text without having to create temp files could be very useful for vanilla ES5, XML, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants