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

Formatter doesn't have access to filename #3596

Open
IvanGoncharov opened this issue Aug 29, 2022 · 31 comments · May be fixed by #5626
Open

Formatter doesn't have access to filename #3596

IvanGoncharov opened this issue Aug 29, 2022 · 31 comments · May be fixed by #5626
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements

Comments

@IvanGoncharov
Copy link

#2942 added formatter option, which is very useful.
I used it prettier but quickly discovered that I have conflicting formatting between CI and my editor.

This happens because prettier change formatting based on filename, e.g. package.json formatted with json-stringify but other *.json files formatted with json parser.
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/index.js#L81-L94

To solve this use case, prettier provides --stdin-filepath, but I can't use it in helix since there is no way to pass the filename into fomatter option.

Having a placeholder of some kind would be great, e.g. $filename.

@IvanGoncharov IvanGoncharov added the C-enhancement Category: Improvements label Aug 29, 2022
@kirawi kirawi added the A-helix-term Area: Helix term improvements label Aug 31, 2022
@kirawi
Copy link
Member

kirawi commented Aug 31, 2022

Previously #2942 did implement passing a file path to the formatter, but it was removed since stdin was deemed sufficient. Maybe this use case will change things.

@kvrohit
Copy link
Contributor

kvrohit commented Sep 9, 2022

I have a similar issue too which would be solved if there was a way to pass in the filename to the formatter command.

@JamesMDunn
Copy link

I also have this issue with prettier. Not sure how to get it to format correctly

@Anthuang
Copy link

You can manually specify the parser for prettier to use to avoid needing the file name, see https://prettier.io/docs/en/options.html#parser.

E.g.:

[[language]]
name = "typescript"
auto-format = true
formatter = { command = "prettier", args = ["--parser", "typescript"] }

@ttys3
Copy link
Contributor

ttys3 commented Oct 14, 2022

@Anthuang this is hard to use, because the cli programmer know better than you.
for prettier, it has flow|babel|babel-flow|babel-ts|typescript|acorn|espree|meriyah|css|less|scss|json|json5|json-stringify|graphql|markdown|mdx|vue|yaml|glimmer|html|angular|lwc parsers

we can not count on a hard coded config to manual calculate the parser to use. this is bad.

@ttys3
Copy link
Contributor

ttys3 commented Oct 14, 2022

I got this problem too.

I think provide the full path filename to the formatter is very important.

some formatters need the full file path to help with location a config in the project (stylua) or use it to determine the file type (prettier),

in neovim we can use vim.api.nvim_buf_get_name(0) to get the current buf file path, but in hexlix seems no way to do so.

for example:

style:

--stdin-filepath Specify the location of the file that is being passed into stdin. Ignored
if not taking in input from stdin. This option is only used to help
determine where to find the configuration file

require("formatter").setup {
	filetype = {
		javascript = {
			-- prettier
			function()
				return {
					exe = "prettier",
					args = { "--stdin-filepath", vim.api.nvim_buf_get_name(0), "--single-quote" },
					stdin = true,
				}
			end,
		},
		lua = {
			-- stylua
			function()
				return {
					exe = "stylua",
					args = { "--search-parent-directories", "--stdin-filepath", vim.api.nvim_buf_get_name(0), "-" },
					stdin = true,
				}
			end,
		},
	}
}

@nestor-custodio
Copy link

Just adding a +1 to this as a Rubocop user. Without a filename, it's impossible to properly configure rules that only apply to (or are only excepted from) certain files and/or directories.

@kirawi
Copy link
Member

kirawi commented Dec 19, 2022

Reverting f7ecf9c and acdce30 should add filename formatting.

@kirawi kirawi added E-easy Call for participation: Experience needed to fix: Easy / not much E-good-first-issue Call for participation: Issues suitable for new contributors E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR labels Dec 19, 2022
filipdutescu added a commit to filipdutescu/helix that referenced this issue Jan 21, 2023
Add support for a current file placeholder in formatter arguments. This
should make it possible to run custom formatters, such as `rome`
(Typescript), `rustfmt` (Rust) and more.

The changes take inspiration from f7ecf9c and acdce30, but instead of
adding a new field to the formatter struct, it works by looking for a
predefined placeholder (`{}`), which should be replaced, if found. This
should allow for better control over arguments of formatters, allowing
both giving it as a standalone arg (`"{}"`) and as part of one (ie
`"--src-file={}"`). The placeholder, `{}`, is used since it is common,
Rust using it frequently.

Closes: helix-editor#3596
Signed-off-by: Filip Dutescu <[email protected]>
@filipdutescu filipdutescu linked a pull request Jan 21, 2023 that will close this issue
filipdutescu added a commit to filipdutescu/helix that referenced this issue Jan 21, 2023
Add support for a current file placeholder in formatter arguments. This
should make it possible to run custom formatters, such as `rome`
(Typescript), `rustfmt` (Rust) and more.

The changes take inspiration from f7ecf9c and acdce30, but instead of
adding a new field to the formatter struct, it works by looking for a
predefined placeholder (`{}`), which should be replaced, if found. This
should allow for better control over arguments of formatters, allowing
both giving it as a standalone arg (`"{}"`) and as part of one (ie
`"--src-file={}"`). The placeholder, `{}`, is used since it is common,
Rust using it frequently.

Closes: helix-editor#3596
Signed-off-by: Filip Dutescu <[email protected]>
filipdutescu added a commit to filipdutescu/helix that referenced this issue Jan 22, 2023
Add support for a current file placeholder in formatter arguments. This
should make it possible to run custom formatters, such as `rome`
(Typescript), `rustfmt` (Rust) and more.

The changes take inspiration from f7ecf9c and acdce30, but instead of
adding a new field to the formatter struct, it works by looking for a
predefined placeholder (`{}`), which should be replaced, if found. This
should allow for better control over arguments of formatters, allowing
both giving it as a standalone arg (`"{}"`) and as part of one (ie
`"--src-file={}"`). The placeholder, `{}`, is used since it is common,
Rust using it frequently.

Closes: helix-editor#3596
Signed-off-by: Filip Dutescu <[email protected]>
filipdutescu added a commit to filipdutescu/helix that referenced this issue Feb 4, 2023
Add support for a current file placeholder in formatter arguments. This
should make it possible to run custom formatters, such as `rome`
(Typescript), `rustfmt` (Rust) and more.

The changes take inspiration from f7ecf9c and acdce30, but instead of
adding a new field to the formatter struct, it works by looking for a
predefined placeholder (`{}`), which should be replaced, if found. This
should allow for better control over arguments of formatters, allowing
both giving it as a standalone arg (`"{}"`) and as part of one (ie
`"--src-file={}"`). The placeholder, `{}`, is used since it is common,
Rust using it frequently.

Closes: helix-editor#3596
Signed-off-by: Filip Dutescu <[email protected]>
@Icantjuddle
Copy link

clang-format would also benefit from being able to pass the filename.

@vanarok
Copy link
Contributor

vanarok commented May 26, 2023

eslint cannot format without stdin-filename

@jokeyrhyme
Copy link

jokeyrhyme commented May 26, 2023

I think this might be more generally-solved by #7105

Or, perhaps, a similar approach to that issue: where there is a general value placeholder system, and an initially-fixed set of values we can refer to from within the TOML configuration file, etc

@the-mikedavis the-mikedavis removed E-good-first-issue Call for participation: Issues suitable for new contributors E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR E-easy Call for participation: Experience needed to fix: Easy / not much labels Aug 19, 2023
@rcorre
Copy link
Contributor

rcorre commented Sep 8, 2023

I've encountered two cases where this would be useful:

  • clang-format-diff needs to know what file you're editing, so it can apply a relevant section of the diff. This is useful when you want to format new changes in a codebase that is not fully formatted.
  • goimports requires a filename, not stdin. I think this is because it needs to know the name of the module when sorting imports. I'm wrong, goimports works over stdin.

@tynanbe
Copy link

tynanbe commented Nov 14, 2023

I'm using this as a workaround.

formatter = { command = "sh", args = ["-c", """
  file=$(
    ps -p "${PPID}" -o "command=" \
      | awk '{ print $NF }'
  )
  exec prettier \
    --stdin-filepath="${file}" \
    --config-precedence=file-override
"""] }

@rcorre
Copy link
Contributor

rcorre commented Nov 14, 2023

@tynanbe that will only work if you ran hx some_file, right? If you open a different file within helix, it won't know what the file is.

FWIW, using source.organizeImports with #6486 (comment) means I no longer need to run goimports as a formatter, but this would still be great for clang-format-diff.

@tynanbe
Copy link

tynanbe commented Nov 14, 2023

@rcorre, indeed, I haven't tested that use case, but I personally don't use Helix in that way, so not a problem for me 😄

storopoli added a commit to storopoli/flakes that referenced this issue Nov 17, 2023
@71GA
Copy link

71GA commented Dec 5, 2023

Usually for clang-format we execute it like this:

clang-format -i --style=file:<configuration file> <source_file>

While <configuration file> is usually always the same and can be hardcoded, <source file> is usually always different and should be absolute path to the file currently being edited in Helix. So we need a file placeholder to a file currently being edited.

@rcorre
Copy link
Contributor

rcorre commented Dec 6, 2023

@71GA you can use plain clang-format with helix currently, as it can communicate over stdin/stdout. clangd also supports formatting if you use that as your language server. Typically I just use clangd for C++ formatting, and clang-format with the --assume-filename flag to format protobuf:

[[language]]
name = "protobuf"
formatter = { command = "clang-format" , args = ["--assume-filename=a.proto"]}

It was specifically clang-format-diff (for partially formatted codebases) that I struggled with.

@foxcroftjn
Copy link

foxcroftjn commented Dec 13, 2023

To address the original issue, it is possible to create a custom language block for a specific set of files. This can be done by adding the following block to your languages.toml file:

[[language]]
name = "package.json" # this can be set to any string that doesn't conflict with other language names
scope = "source.json"
roots = ["package.json"]
file-types = ["package.json", "package-lock.json", "composer.json"]
formatter = { command = "npx", args = ["prettier", "--stdin-filepath", "package.json"] }
language-servers = ["vscode-json-language-server"]
auto-format = true
grammar = "json"

To get syntax highlighting to work, you also need to copy (or symlink) the runtime/queries/json directory to runtime/queries/package.json.

This seems like the cleanest workaround for now (to me), but I agree that it would be best if helix supported passing the filename to the formatter.

@bluthej
Copy link

bluthej commented Jan 9, 2024

I have the same problem in Python with Ruff when trying to sort imports.
First-party imports are incorrectly categorized as third-party if stdin is used without the --stdin-filename option.
This leads to inconsistencies between running Ruff as an auto-format command and as a code action.

@Bobrokus
Copy link

Bobrokus commented Feb 1, 2024

+1
In my case, the path is required by PowerShell-Beautifier

@akhansari
Copy link

This is a mandatory feature, especially when the formatter config file is not at the root.

@zjr
Copy link

zjr commented Mar 16, 2024

@akhansari pestering the maintainers isn’t going to motivate anyone to get it done. If you feel it’s important it might be wise to focus your efforts on writing a PR instead.

@akhansari
Copy link

akhansari commented Mar 16, 2024

Just got mugged...

@zjr It wasn't my intention. I wanted to contribute by giving a feedback as a user and to highlight the importance of this feature for formatters usually based on local config files. Nothing else.

And if I had enough time and if I knew rust well, it would have been with pleasure. Could you please give the benefit of doubt next time and not make hasty conclusions. It's toxical for the community.
Although English is not main language of many contributers and they can miss some nuances.

@g5becks
Copy link

g5becks commented Jul 19, 2024

Resharper command line tools

https://www.jetbrains.com/help/rider/CleanupCode.html

and Csharpier

https://csharpier.com/docs/CLI

Also, a file path or a solution file is required when working with C#.

@ttys3
Copy link
Contributor

ttys3 commented Jul 19, 2024

from 2022 to now, been almost 2 years, I do not know why this has not been resolved?

If the dev team does not support this feature, then just close this issue is better.

If does, someone can fire a PR

@boydkelly
Copy link

Is this the problem I have with lua? I have tried:

name = "lua"
auto-format = true
formatter = { command = "stylua", args = ["--stdin-filepath"] }
````
But its saying I also need the file name as an argument.   I have not found any documentation on how to set this up...

@benjamineskola
Copy link

@boydkelly

formatter = { command = "stylua", args = ["--stdin-filepath"] }

In cases like this you can often use a fake filename. For example, prettier uses the filename to determine the filetype, but in most cases only really cases about the ending, so you can do args = ["--stdin-filepath", "dummy.lua"]. Because it's actually processing the data from stdin and printing it to stdout, it doesn't matter whether that's a real filename or not.

(It's not perfect: if the formatter has different rules based on the real path then you can't solve that this way AFAIK.)

@boydkelly
Copy link

It gives me the error: 2024-07-19T19:10:00.041 helix_view::document [ERROR] Formatter error: error: no files provided
And if i provide no filename argument i get the output of stylua asking for the argument to --stdin-filepath

@kirawi
Copy link
Member

kirawi commented Jul 19, 2024

It's covered under their readme https://github.com/JohnnyMorganz/StyLua?tab=readme-ov-file#usage It should be stylua -

@boydkelly
Copy link

It's covered under their readme https://github.com/JohnnyMorganz/StyLua?tab=readme-ov-file#usage It should be stylua -

Thank you! So what works is:
formatter = { command = "stylua", args = ["-"] }

@bluthej
Copy link

bluthej commented Dec 19, 2024

I have another use case for giving access to the filename to the formatter. In Python we can have stub files with the pyi extension which are not formatted like Python modules with the py extension. Currently, my formatter config for Python looks like this:

formatter = { command = "bash", args = ["-c", "ruff format --quiet -"] }

But since the files are just passed to ruff via stdin it has no way to distinguish between py and pyi files, so the latter are not formatted correctly.

I'd be willing to give it a shot and open a PR, unless the maintainers do not want this feature implemented? I would love to have just an OK from a maintainer before I start spending time on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.