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

Apply exclude_files to files absolute paths #356

Conversation

danielfinke
Copy link
Contributor

When calling erlfmt with absolute paths for the files to format, the excludes don't apply, even if the resolved paths of the excludes would match the files to format.

Resolve the paths of the excludes based on the current working directory and add them to the excludes.

This issue was observed when using the
VS Code Erlang Formatter extension. The extension invokes rebar3 fmt with the full path of the file to format.

When calling `erlfmt` with absolute paths for the files to format, the
excludes don't apply, even if the resolved paths of the excludes would
match the files to format.

Resolve the paths of the excludes based on the current working directory
and add them to the excludes.

This issue was observed when using the
[VS Code Erlang Formatter extension](https://marketplace.visualstudio.com/items?itemName=szTheory.erlang-formatter).
The extension invokes `rebar3 fmt` with the full path of the file to
format.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 17, 2024
Copy link
Member

@awalterschulze awalterschulze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, what do you think @michalmuskala ?

Copy link
Member

@michalmuskala michalmuskala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern about windows support.

The second concerns would be with symlinks. I'm not sure it would all work correctly in this case.

Wouldn't an alternative of rather than checking for file equality to check for prefixes work?

src/erlfmt_cli.erl Outdated Show resolved Hide resolved
This handles Windows' `volumerelative` path types by properly resolving
them based on the "per-drive" working directory.

Also, resolve the input file absolute paths. This is new functionality;
if you were previously to use a file path like `a/../b`, that would have
been directly matched against the excludes. As a result, an exclude of
`b` would not have applied. This change makes sure relative path
resolution is consistent between `files`/`exclude_files`.

To avoid displaying different paths from what was requesting by config
or CLI options (e.g. when using the verbose flag), use a map of absolute
paths -> original paths and return the original paths from
`erlfmt_cli:set_difference/2` after applying excludes.
@danielfinke
Copy link
Contributor Author

danielfinke commented Aug 28, 2024

@awalterschulze @michalmuskala ready for further review 🙂

I've addressed Windows support by switching to using filename:absname/2 for abspath resolution.

The change also makes sure files/exclude_files consistently resolve relative paths in case of double dots/parent directory segments. Note that previously, erlfmt didn't resolve these - e.g. a/../b and b were distinct. I had already resolved these for the absolute excludes in the first commit. Now it applies to all paths for both files/exclude_files.

I intentionally avoided any symlink resolution. The existing behaviour of erlfmt treats symlinks as distinct files. e.g.

$ echo "   -module(a)." > a.erl
$ ln -s a.erl b.erl
$ rebar3 fmt --verbose --exclude-files "a.erl" b.erl
Formatting b.erl
-module(a).

a.erl is skipped, because the b.erl symlink is not followed.

I think this makes sense and is consistent with most treatment of symlinks in my experience. One example is the script name argument when running a bash script:

$ echo 'echo $0' > test.sh
$ chmod +x test.sh
$ ln -s test.sh test2.sh
$ ./test.sh
./test.sh
$ ./test2.sh
./test2.sh

Unless the script explicitly follows its script name symlink, it thinks it is a unique script.

@michalmuskala michalmuskala merged commit c2286a8 into WhatsApp:main Sep 2, 2024
8 checks passed
@michalmuskala
Copy link
Member

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants