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

Support custom variable to indicate current filename #13

Open
terlar opened this issue Jul 20, 2019 · 5 comments
Open

Support custom variable to indicate current filename #13

terlar opened this issue Jul 20, 2019 · 5 comments

Comments

@terlar
Copy link

terlar commented Jul 20, 2019

I like the simple implementation but wonder if it would make sense to interpolate some variable as current buffer filename. Of course this comes with the complication, should it be only the filename, the full path or the project local one. Perhaps even the possibility to provide a custom function to deterimine the file-name if it is too complicated for the project.

The reason I am asking is that some formatters want the file name and act differently depending on the filename, e.g. prettier/eslint. Right now I have solved this by creating different formatters for the different file-extensions, where I just put a static file, such as f.ts. However I have learned with slow formatters such as ESlint that it makes it really slow, I guess it is using the actual filename to determine some cache.

Sample timings:

$ time -p eslint_d --stdin --stdin-filename test/unit/services/attachment.test.ts --fix-to-stdout < test/unit/services/attachment.test.ts
real 0.25
user 0.05
sys 0.01

$ time -p eslint_d --stdin --stdin-filename f.ts --fix-to-stdout < test/unit/services/attachment.test.ts
real 4.88
user 0.05
sys 0.01

So in order to make this formatter useable I would need to provide the filename.

My current formatter looks like:

(reformatter-define typescript-format
  :program "eslint_d"
  :args '("--stdin" "--stdin-filename=f.ts" "--fix-to-stdout")))

What would be nice:

(reformatter-define typescript-format
  :program "eslint_d"
  :args '("--stdin" "--stdin-filename=%f" "--fix-to-stdout")))

or something like this using the symbol to figure out what to do:

(reformatter-define typescript-format
  :program "eslint_d"
  :args '("--stdin" "--stdin-filename" file "--fix-to-stdout")))
@wbolster
Copy link
Contributor

wbolster commented Nov 3, 2019

the :program and :args parameters are actually lisp forms that will be executed, not just data.

this means you can hook custom logic into it. this is essential for some formatters.

see my package python-black.el for a practical example.

@wyuenho
Copy link
Contributor

wyuenho commented May 25, 2020

@wbolster This will be good to document.

@purcell
Copy link
Owner

purcell commented May 27, 2020

So in order to make this formatter useable I would need to provide the filename.

The problem with this approach is that you shouldn't have to save the file to reformat it, and it should be possible to reformat a buffer that is not backed by a file. So in that case, if a formatter needs to be given the path to a file, it should be the path of a temporary file to which the buffer contents have been written. And then there comes the question of where to write that temporary file, e.g. in a temp dir or the current dir, and certain choices in that regard can upset linters. For example, a linter might search directories upwards from the input file to find its config file. This is why flymake has a bunch of different functions for making temporary copies of files in various ways. We might consider allowing reformatter-define to be passed such a function, but clearly it requires a little thought.

Related to this, I note that in the python-black code you linked to, @wbolster, if the current buffer is backed by a file and you make some modifications then call black-buffer before you save the buffer, then you will lose those modifications. So in this case you shouldn't ever be using (buffer-file-name).

@purcell
Copy link
Owner

purcell commented May 27, 2020

I note that in the python-black code you linked to

Sorry, ignore me: you're not passing the filename itself... I think I'm just tired today.

@wyuenho
Copy link
Contributor

wyuenho commented May 27, 2020

@purcell eslint only requires a file name for stdin when there are linting rules that needs to read the file name, this is a problem with formatters that mixes linting and formatting in the same program. Eslint doesn't use the file name to read off a file. This is a separate issue from #18. @wbolster 's approach will enable eslint_d and eslint to work as in #25 . I think we just need to document it if it becomes a recurrent issue.

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

No branches or pull requests

4 participants