-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Ignore symlinks when formatting #57
Conversation
Ah, whoops, I broke |
This separates data and functionality better.
Before this, symlinks were replaced with a formatted file rather than formatting the file behind the symlink. The choice was between ignoring symlinks and following them. Following symlinks means potentially formatting files in an unexpected location which seems much worse than needing to wrap the arguments in a `readlink -f` when symlinks are otherwise ignored.
3e975cd
to
5c2281a
Compare
That should fix that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, but I do insist on using safe-exceptions
. (Especially, given that it’s already a dependency anyway.)
@@ -9,6 +9,7 @@ | |||
module Main where | |||
|
|||
import Control.Concurrent (Chan, forkIO, newChan, readChan, writeChan) | |||
import Control.Exception (IOException, try) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, only ever use Control.Exception.Safe
from safe-exceptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also apparently it's in a wrong commit 🙄
filterTarget :: (Target -> IO Result) -> Target -> IO Result | ||
filterTarget operation StdioTarget = operation StdioTarget | ||
filterTarget operation (FileTarget path) = do | ||
isSymlink <- try $ pathIsSymbolicLink path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also catch async exceptions which you don’t want (this probably doesn’t matter in this specific case, but it’s better to place safe and future-proof). Use try
for safe-exceptions
.
One option might be to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything substantial to add. Using safe-exceptions
sounds like a good practice, but some people don't like it (not me) and for IOException
it should not matter, so I don't really care if you change that.
I agree with using |
We plan to deprecate the directory mode directly in nixfmt and instead encourage use of treefmt. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-05-28/46126/1 |
See #151 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-08-06/50222/1 |
Before this, symlinks were replaced with a formatted file rather than
formatting the file behind the symlink. The choice was between ignoring
symlinks and following them. Following symlinks means potentially
formatting files in an unexpected location which seems much worse than
needing to wrap the arguments in a
readlink -f
when symlinks areotherwise ignored.
Fixes #56.