-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Correctly handle last lines with spaces #79
Conversation
Hmm, this introduces some unnecessary simple strings:
|
c8c5148
to
ed03940
Compare
Fixed now. |
@kirelagin Could you get this reviewed and merged? |
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.
The whitespace detection lacks tabs, and some derivations use it. This might produce incorrect output.
|
||
isInvisibleLine :: [StringPart] -> Bool | ||
isInvisibleLine [] = True | ||
isInvisibleLine [TextPart t] = Text.null $ Text.strip t |
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.
Why not isSpaces
, like in the rest of the code?
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 function is specifically to catch an edge case where strings are better represented as a simple ""
string, rather than an indented ''''
string. It is not about matching the behavior of Nix. I added this because nixfmt used to turn things like "\n\n"
and "\n "
into indented strings.
@@ -78,3 +79,6 @@ dropCommonIndentation unstrippedLines = | |||
in case commonIndentation (filter (/=empty) strippedLines) of | |||
Nothing -> map (const empty) strippedLines | |||
Just indentation -> map (fromMaybe empty . stripPrefix indentation) strippedLines | |||
|
|||
isSpaces :: Text -> Bool | |||
isSpaces = Text.all (==' ') |
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.
What if there are tabs? NixOS/nix#2911
I suggest replacing it with Text.all Data.Char.isSpace
(it even is already imported!).
isSpaces = Text.all (==' ') | |
isSpaces = Text.all isSpace |
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.
Nix handles lines consisting entirely of spaces different from lines that contain other kinds of whitespace. This function is intended to match that behavior but it could probably be better documented. I got stuck on finding good names for lines without any characters vs lines consisting of only but at least one space vs lines that could be either.
Fixes #78