home-manager icon indicating copy to clipboard operation
home-manager copied to clipboard

zsh: `localVariables` should take a `dag`

Open dbaynard opened this issue 3 years ago • 5 comments

https://github.com/nix-community/home-manager/blob/94780dd888881bf35165dfdd334a57ef6b14ead8/modules/programs/zsh.nix#L396-L403

I hit this when creating one variable (FZF_CTRL_R_OPTS) which referenced another (FZF_CTRL_R_PREVIEW). The former was always defined first, as the current code uses lib.mapAttrsToList, which uses builtins.attrNames, which sorts the variables alphabetically.

(My workaround is to rename the latter variable, which I can do here.)

I’m happy to implement this change, if it is desirable.

dbaynard avatar May 22 '22 01:05 dbaynard

Actually, I’ve just noticed that escaping is broken, too — I can't investigate right now, but I think the problem is toZshValue.

https://github.com/nix-community/home-manager/blob/db00b39a9abec04245486a01b236b8d9734c9ad0/modules/lib/zsh.nix#L5-L13

I see there is actually no escaping, here; this is probably not what users expect. I’ll ponder this, and perhaps raise an issue if I feel strongly enough to propose a change.

dbaynard avatar May 22 '22 01:05 dbaynard

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

  • If this is resolved, please consider closing it so that the maintainers know not to focus on this.
  • If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.
If you are not the original author of the issue

  • If you are also experiencing this issue, please add details of your situation to help with the debugging process.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.
Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

stale[bot] avatar Aug 31 '22 01:08 stale[bot]

If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Great, this is what I wanted to know, before starting to work on a PR. I might tag a maintainer if I find it tricky to maintain backwards compatibility.

dbaynard avatar Aug 31 '22 08:08 dbaynard

I would recommend using Nix features instead, using a dag is overkill in this case.

For example, something like

localVariables = let FZF_CTRL_R_PREVIEW = "whatever"; in {
  inherit FZF_CTRL_R_PREVIEW;
  FZF_CTRL_R_OPTS = "something with ${FZF_CTRL_R_PREVIEW}";
};

rycee avatar Aug 31 '22 09:08 rycee

Thank you — this is subtly different to what I was trying to do, but it would basically work. For my use case, renaming the variable works better, though.

In your example, FZF_CTRL_R_OPTS is set to something with whatever, whereas I want it to be set to something with $FZF_CTRL_R_PREVIEW. The latter form means I can just copy and paste the generated code when I want to change FZF_CTRL_R_PREVIEW temporarily, to reflect a reassigned FZF_CTRL_R_OPTS.

If a dag is overkill, I wonder if there's a way to preserve the order, that is worthwhile?

dbaynard avatar Aug 31 '22 12:08 dbaynard

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

  • If this is resolved, please consider closing it so that the maintainers know not to focus on this.
  • If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.
If you are not the original author of the issue

  • If you are also experiencing this issue, please add details of your situation to help with the debugging process.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.
Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

stale[bot] avatar Dec 02 '22 04:12 stale[bot]

I'll close this; if I feel the zsh variable escaping issue is a problem, I'll open a new issue for that.

dbaynard avatar Dec 18 '22 16:12 dbaynard