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

Correctly handle last lines with spaces #79

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Conversation

Lucus16
Copy link
Contributor

@Lucus16 Lucus16 commented Sep 19, 2021

Fixes #78

@Lucus16
Copy link
Contributor Author

Lucus16 commented Sep 19, 2021

Hmm, this introduces some unnecessary simple strings:

-      description = ''
-        A macro to generate structures which behave like bitflags.
-      '';
+      description =
+        "A macro to generate structures which behave like bitflags.\n";

@Lucus16 Lucus16 force-pushed the fix-string-indentation branch from c8c5148 to ed03940 Compare September 19, 2021 13:18
@Lucus16
Copy link
Contributor Author

Lucus16 commented Sep 19, 2021

Fixed now.

@Lucus16 Lucus16 requested a review from kirelagin September 23, 2021 16:12
@Lucus16
Copy link
Contributor Author

Lucus16 commented Sep 23, 2021

@kirelagin Could you get this reviewed and merged?

Copy link

@Heimdell Heimdell left a 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

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?

Copy link
Contributor Author

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 (==' ')

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!).

Suggested change
isSpaces = Text.all (==' ')
isSpaces = Text.all isSpace

Copy link
Contributor Author

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.

@Lucus16 Lucus16 merged commit 324c4cd into master Dec 2, 2021
@Lucus16 Lucus16 deleted the fix-string-indentation branch December 2, 2021 16:10
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 this pull request may close these issues.

nixfmt breaks trailing spaces in strings when doing "" to '''' conversion
2 participants