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

3 participants
@FichteFoll
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.

@braver braver added the enhancement label Feb 5, 2018

@kaste

This comment has been minimized.

Show comment
Hide comment
@kaste

kaste Feb 9, 2018

Contributor

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

Contributor

kaste commented Feb 9, 2018

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

@kaste

This comment has been minimized.

Show comment
Hide comment
@kaste

kaste Feb 9, 2018

Contributor

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

Contributor

kaste commented Feb 9, 2018

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

@braver

This comment has been minimized.

Show comment
Hide comment
@braver

braver Feb 9, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@kaste

kaste Feb 26, 2018

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@FichteFoll

FichteFoll Feb 26, 2018

Contributor

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.)

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@kaste

kaste Feb 26, 2018

Contributor

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}?

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}?

@kaste kaste referenced a pull request that will close this issue Feb 26, 2018

Open

Mark 'file-only-linters' using `${real_file}` in cmd #1022

@FichteFoll

This comment has been minimized.

Show comment
Hide comment
@FichteFoll

FichteFoll Feb 26, 2018

Contributor

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?

Contributor

FichteFoll commented Feb 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@kaste

kaste Feb 27, 2018

Contributor

I'll take ${temp_file}.

Contributor

kaste commented Feb 27, 2018

I'll take ${temp_file}.

@braver

This comment has been minimized.

Show comment
Hide comment
@braver

braver Mar 3, 2018

Member

discussion moved to #1073

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