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

Allow tab usage in indented strings #2911

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Allow tab usage in indented strings #2911

wants to merge 3 commits into from

Conversation

cslamber
Copy link

@cslamber cslamber commented Jun 3, 2019

There are a fair number of derivations in nixpkgs that seem to unintentionally use tabs in their expressions instead of spaces (evilwm, gitit, animbar, ocaml-expat), and right now spaces are the only indentation recognized. This change allows tabs to be used by having whatever whitespace character appears first at the start of lines as the character used for indentation. No assumptions need to be made about tab configuration because this does not support mixed tabs and spaces. This would also allow people who prefer tabs in their local derivations to use that instead.

The only place where mixing is acceptable is with the final ignored line, as some of the packages specified above have spaces indenting their final lines, which seem to be intended to be ignored.

This is my first PR, so feel free to modify or critique it however you want.

@edolstra
Copy link
Member

edolstra commented Jun 3, 2019

I think it's better to be opinionated and discourage/disallow the use of tabs. If there are places in Nixpkgs that use tabs, they should be fixed.

@7c6f434c
Copy link
Member

7c6f434c commented Jun 3, 2019 via email

@edolstra
Copy link
Member

edolstra commented Jun 3, 2019

To ensure consistency.

@cslamber
Copy link
Author

cslamber commented Jun 3, 2019

I know a number of languages that recommend using spaces to indent, but none that outright require it.

As far as I'm aware, this is the only place where the nix language cares about whitespace. I personally like to (ab)use elastic tabstops, so having to switch only when writing indented strings in personal derivations is a bit annoying. It isn't breaking anything, but it does make writing .nix feel a little awkward compared to other languages. Maybe some sort of opinionated nix fmt instead would work to ensure consistency across nixpkgs? It seems like some maintainers are already using a nix formatter, since that would explain the packages mentioned above using tabs in indented strings but spaces everywhere else (most tellingly in the ignored last line of indented strings).

Plus, since this would make the behavior of indented strings work consistently with tabs, a formatter would be able to replace tabs with spaces in indented strings without changing behavior.

@cslamber cslamber closed this Jun 3, 2019
@cslamber cslamber reopened this Jun 3, 2019
@cslamber
Copy link
Author

cslamber commented Jun 3, 2019

Somehow my cursor hit the close button when I closed my laptop. Reopened

@7c6f434c
Copy link
Member

7c6f434c commented Jun 3, 2019

@edolstra could you please expand a bit? Because in another place, you opposed the simplest available way to ensure syntactic consistency…

@edolstra
Copy link
Member

edolstra commented Jun 3, 2019

@7c6f434c On the contrary, I'm all in favor of consistent use of URLs.

@7c6f434c
Copy link
Member

7c6f434c commented Jun 3, 2019

Well, at least with spaces and tabs we hopefully won't get a limitation that makes the encouraged option inevitably suboptimal for the most frequent use case.

@scintill
Copy link

If tabs aren't stripped, I suggest updating

<para>Indented strings are primarily useful in that they allow
multi-line string literals to follow the indentation of the
enclosing Nix expression, and that less escaping is typically

, since tabs are indentation, but multiline literals in tab-formatted file don't work as I would expect from reading that.

Maybe even make tabs a syntax error, if using them is going to subtly change the meaning of expressions. (I just spent a few minutes figuring out why two identical-looking pieces of code had different outputs...)

@wmertens
Copy link
Contributor

Nix is also used in user configurations, that don't need to adhere to nixpkgs guidelines. So for a better user experience, I would kindly request that this gets merged.

For consistency in nixpkgs, the CI tools can check that edited files contain only spaces for indentation.

@kevincox
Copy link

This has been confusing me for years because I was wondering why the docs said that indentation would be stripped however I was still seeing indentation in my output. This should definitely fixed. Enforcing an indentation style on nixpkgs can be done with a simple linter (and be more effective) however there is no reason to prevent tabs for personal nix configs especially when it is a source of confusion.

@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@wmertens
Copy link
Contributor

How about outputting a warning that tabs should not be used in nix definition files, for consistency?

@stale stale bot removed the stale label Feb 13, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Aug 13, 2021
@scintill
Copy link

scintill commented Aug 14, 2021 via email

@stale stale bot removed the stale label Aug 14, 2021
@JojOatXGME
Copy link

JojOatXGME commented Aug 14, 2021

For reference, issue #3759 is also about this topic. Note that other line terminators (i.e. CRLF) also seem to “break” indented strings.

@frogamic
Copy link
Contributor

Is this discussion still active? I do not understand why only spaces would be stripped when there is a well understood concept of "Whitespace" (see every regex dialect), I don't want nixpkgs strict style guidelines to be forced onto me in my own personal repos where I choose to use tabs for my own reasons, I should be able to do that. Furthermore if it is to be arbitrarily limited to spaces only, the doco should be updated because stating that "whitespace" is removed is misleading.

@stale
Copy link

stale bot commented Jun 12, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 12, 2022
@scintill
Copy link

Not stale AFAIK

@fricklerhandwerk fricklerhandwerk added the language The Nix expression language; parser, interpreter, primops, evaluation, etc label Sep 9, 2022
@stale stale bot removed the stale label Sep 9, 2022
@ghost

This comment was marked as off-topic.

@@ -41,8 +41,9 @@ let
'';

s6 = ''
Tabs are not interpreted as whitespace (since we can't guess
what tab settings are intended), so don't use them.
Tabs are only interpreted as whitespace when they are
Copy link

Choose a reason for hiding this comment

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

whitespace when should be whitespace when. No need for two spaces.

@fricklerhandwerk fricklerhandwerk added the feature Feature request or proposal label Mar 3, 2023
@edolstra edolstra added the breaking Changes we can't make without breaking old expressions, changing hashes, etc label Jul 14, 2023
@Ericson2314
Copy link
Member

  • Can't do in its current form because it would be a breaking language change, and we can't do those until [RFC 0137] Nix language versioning rfcs#137 is resolved

  • We can however do a warning on tabs, so

    • users are encouraged to match the rest of the ecosystem

    • users are informed that tab indentation in multiline string indents will have surprising behavior

@kevincox
Copy link

kevincox commented Jul 14, 2023

If we do add a warning we need to find a way to make it relatively subtle. Probably would need to be dismissable. The last thing I want is some log noise during every Nix evaluation.

I do agree that if a versioning system is likely to land it would be a good way to roll this out.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-07-14-nix-team-meeting-minutes-71/30472/1

@JojOatXGME
Copy link

JojOatXGME commented Jul 16, 2023

We can however do a warning on tabs

And please also add a warning on CRLF (and CR), since they cause the same problem. While it is probably less common to use CRLF than tabs in the Linux ecosystem, I guess adding this check in addition to the check for tabs shouldn't be that complicated. [EDIT: Also, if you somehow end up with CRLF, it is often less visible than tabs. Many editors will just start using CRLF for each line termination if they find one occurrence in the file. Some editors1 will not even show you an indication which line termination they are using.]

FYI, I don't know why CRLF breaks indented string since the CR (\r) is at the end of the line, not at the beginning, but it does.

Footnotes

  1. I think I even experienced vim not showing me any indication that it is using CRLF for a file while using the default configuration of some Linux distribution, but not sure.

@buckley310
Copy link

It's probably a good thing to eliminate "surprising behavior" where possible, rather than warn about it IMO. If RFC 137 succeeds, that sounds like the better route to me.

I don't think this will threaten ecosystem consistency, given that nixpkgs and every nix formatter I have seen uses spaces.

@roberth
Copy link
Member

roberth commented Sep 3, 2023

Marked as draft because this is either blocked on RFC 137 for supporting breaking language changes, or this could be converted to a change that warn appropriately when such tabs are encountered.

@buckley310
Copy link

buckley310 commented Oct 27, 2023

RFC 137 did not pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Changes we can't make without breaking old expressions, changing hashes, etc feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet