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

Introduce a separate attribute for declaring a linter to only operate on the exact files #954

Open
FichteFoll opened this issue Feb 4, 2018 · 9 comments

Comments

@FichteFoll
Copy link
Contributor

FichteFoll commented Feb 4, 2018

Currently, you can declare a linter as a "file-only" linter setting tempfile_suffix to '-'. Its effects are that no temporary file is generated while modifying the view and instead the linter is only run on the saved file with its original path.

This API is quite unintuitive and should be replaced with a separate attribute just for that feature.

Related: #810.

@kaste
Copy link
Contributor

kaste commented Feb 9, 2018

Related #244 ? Isn't specifying a suffix yagni bc we can just use the filename suffix?

@kaste
Copy link
Contributor

kaste commented Feb 9, 2018

T.i. remove tempfile_suffix and instead expose lint_mode as a linter setting.

@braver
Copy link
Member

braver commented Feb 9, 2018

bc we can just use the filename suffix

A buffer doesn't have to be associated with a file, e.g. it hasn't been saved yet. Also, if you lint embedded code by taking it out of the surrounding file and putting it in a temporary file, you can't use the original file's suffix.

That said, especially in a unix-like context, programs that work on a file rarely care about that file's name. If a program receives a file to lint, it should usually just lint it unless the linter's author has been smoking something. I don't think you need a temp file suffix at all in 99% of cases. I do seem to remember that some php linter or other will not lint files that don't have .php at the end of their file name. So we may still need a tempfile_suffix at least as an optional thing for stupid stubborn linters.

expose lint_mode as a linter setting

This is an all around good idea.

@kaste
Copy link
Contributor

kaste commented Feb 26, 2018

Okay, just to summarize what I know:

# exactly three modes
mode = STDIN | TEMPORARY_FILE | REAL_FILE

# which have different `cmd`s. E.g.
cmd = 'eslint ${args} --stdin'  # variation cmd = 'eslint --stdin-filename ${file} --'
cmd = 'eslint ${args} ${temporary_file}'
cmd = 'eslint ${args} ${real_file}'

But we cannot deduce the mode from 'cmd' unless we introduce an alias ${real_file} for ${file_name}.

EDITED to make it proposal like, not just a thought

@FichteFoll
Copy link
Contributor Author

FichteFoll commented Feb 26, 2018

Currently ${tmpfilename} only refers to a tempfile in the temporary file lint mode (and isn't replaced otherwise) and ${file_name} always points to the "real file".
I presume your proposal is to make ${real_file} represent the current ${file_name} behavior and let ${file_name} auto-adjust based on the lint mode? Sounds good to me. By the way, the variable naming scheme should be unified, so ${file} and ${temporary_file} instead of the current names sound good.

Maybe support cmd overrides for each mode? The mypy linter could use this since it uses a different command switch to support temporary file linting in a project context. Especially for linters accepting stdin.
(Note that STDIN and TEMPORARY_FILE are exclusive and the former would be preferred.)

@kaste
Copy link
Contributor

kaste commented Feb 26, 2018

Dude, I forgot that the Sublime variable is actually ${file}. Why didn't you say that as we discussed this in the issue.

Also: ${temporary_file} or ${tmp_file}?

@FichteFoll
Copy link
Contributor Author

Either is fine with me, so I guess pick the shorter one? "tmp" is a widely-understood abbreviation. Alternatively, ${temp_file}, which is even more clear?

@kaste
Copy link
Contributor

kaste commented Feb 27, 2018

I'll take ${temp_file}.

@braver
Copy link
Member

braver commented Mar 3, 2018

discussion moved to #1073

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

Successfully merging a pull request may close this issue.

3 participants