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

cmd/shfmt: document how --filename relates to EditorConfig support #1055

Closed
tcrawford-figure opened this issue Feb 1, 2024 · 6 comments · Fixed by #1058
Closed

cmd/shfmt: document how --filename relates to EditorConfig support #1055

tcrawford-figure opened this issue Feb 1, 2024 · 6 comments · Fixed by #1058

Comments

@tcrawford-figure
Copy link

tcrawford-figure commented Feb 1, 2024

I'm imagining something like the following:

shfmt --editorconfig-path=../relative/path/to/.editorconfig foo.sh
shfmt --editorconfig-path=/absolute/path/to/.editorconfig foo.sh

This becomes useful when integrating shfmt into other applications/plugins. Please see the following PR, if interested, for Gradle plugin integration: diffplug/spotless#2031.

A different flag name can be used than what I provided, but this at least gives the gist of what I'm looking for.

@mvdan
Copy link
Owner

mvdan commented Feb 10, 2024

Not sure I understand; the entire point of editorconfig files is that they apply to the files relative to their location. When would you want to use this flag?

@tcrawford-figure
Copy link
Author

@mvdan There seems to be some potentially unexpected behavior with how .editorconfig is respected.

Take the following .editorconfig for example:

root = true

[*]
charset = utf-8

[*.sh]
indent_style = space
indent_size = 2
space_redirects = true
switch_case_indent = true

When I have a file named foo.sh with the following contents:

#!/usr/bin/env bash

function foo()
{
  if [ -x $file ]
  then
myArray=(item1 item2 item3)
  elif [ $file1 -nt $file2 ]
  then
    unset myArray
          else
    echo "Usage: $0 file ..."
  fi
}

for (( i = 0; i <   5; i++  ))
do
      read -p r
print -n $r
                    wait $!
done

I can successfully format this file, with .editorconfig respected, by invoking:

shfmt foo.sh

However, if I instead add the contents of foo.sh to stdin, the .editorconfig is not respected:

shfmt < foo.sh

Given this, what I have noticed is if I provide a --filename foo.sh to shfmt, the .editorconfig is respected:

shfmt < foo.sh --filename foo.sh

I'm not sure if this is a known feature/issue/limitation? If it is, I think that an update to the man page that mentions that this flag is required when providing content to stdin in order for the nearest .editorconfig to be respected would be a good change.

So this is where my original request came from for having an --editorconfig-path flag.
It may not be necessary and the --filename flag may be the preferred method to fix this.

@mvdan
Copy link
Owner

mvdan commented Feb 13, 2024

That is all working as intended. When you pass in a file via stdin, the filename is not known. We added the filename flag precisely for this purpose (and for related others such as error positions).

@mvdan mvdan closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2024
@tcrawford-figure
Copy link
Author

@mvdan Thanks for confirming! Would documentation in the man page regarding this requirement be worth adding?

@mvdan
Copy link
Owner

mvdan commented Feb 13, 2024

Oops, this was indeed an undocumented fact. Opening a PR.

@mvdan mvdan reopened this Feb 13, 2024
@mvdan mvdan changed the title Add editorconfig flag for custom editorconfig path cmd/shfmt: document how --filename relates to EditorConfig support Feb 13, 2024
mvdan added a commit that referenced this issue Feb 13, 2024
EditorConfig files are found based on a script's absolute path,
and the EditorConfig patterns are usually filename-based as well.

When formatting standard input, there is no known filename or path.
Sometimes there's a need to format a stream of bytes in memory
without having to place them on a file, which is why we support stdin.

We added the --filename flag for this purpose a long time ago,
and the tests already verified this fact - we just hadn't documented it.

While here, also add a test case for absolute paths.

Fixes #1055.
@tcrawford-figure
Copy link
Author

@mvdan perfect! Thank you!

mvdan added a commit that referenced this issue Feb 13, 2024
EditorConfig files are found based on a script's absolute path,
and the EditorConfig patterns are usually filename-based as well.

When formatting standard input, there is no known filename or path.
Sometimes there's a need to format a stream of bytes in memory
without having to place them on a file, which is why we support stdin.

We added the --filename flag for this purpose a long time ago,
and the tests already verified this fact - we just hadn't documented it.

While here, also add a test case for absolute paths.

Fixes #1055.
mvdan added a commit that referenced this issue Feb 13, 2024
EditorConfig files are found based on a script's absolute path,
and the EditorConfig patterns are usually filename-based as well.

When formatting standard input, there is no known filename or path.
Sometimes there's a need to format a stream of bytes in memory
without having to place them on a file, which is why we support stdin.

We added the --filename flag for this purpose a long time ago,
and the tests already verified this fact - we just hadn't documented it.

While here, also add a test case for absolute paths,
and make the man page flags consistently use double dashes.

Fixes #1055.
mvdan added a commit that referenced this issue Feb 13, 2024
EditorConfig files are found based on a script's absolute path,
and the EditorConfig patterns are usually filename-based as well.

When formatting standard input, there is no known filename or path.
Sometimes there's a need to format a stream of bytes in memory
without having to place them on a file, which is why we support stdin.

We added the --filename flag for this purpose a long time ago,
and the tests already verified this fact - we just hadn't documented it.

While here, also add a test case for absolute paths,
and make the man page flags consistently use double dashes.

Fixes #1055.
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

Successfully merging a pull request may close this issue.

2 participants