Skip to content

xdg-user-dirs: allow paths and define sessionVariables#2757

Merged
berbiche merged 1 commit intonix-community:masterfrom
ilkecan:xdg-user-dirs-improvements
Apr 5, 2022
Merged

xdg-user-dirs: allow paths and define sessionVariables#2757
berbiche merged 1 commit intonix-community:masterfrom
ilkecan:xdg-user-dirs-improvements

Conversation

@ilkecan
Copy link
Copy Markdown
Contributor

@ilkecan ilkecan commented Feb 24, 2022

Changed the type of the user directory options from str to either str path(coercedTo path toString str) to allow using path values.

The related session variable is defined for the default and the extra user directories now.

Closes #2694

Description

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@ilkecan ilkecan marked this pull request as ready for review February 24, 2022 13:48
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

AFAIK, the reason paths are not allowed is because it would lead to the entire path being imported in the nix store or prevent building on Flake configurations (Flake's can only refer to paths available in the git index).

@ilkecan ilkecan force-pushed the xdg-user-dirs-improvements branch from f4f26c0 to 76c2e63 Compare February 25, 2022 12:02
@ilkecan
Copy link
Copy Markdown
Contributor Author

ilkecan commented Feb 25, 2022

The paths being copied to the Nix store was not a problem because currently there isn't any antiquotation happening with those values. But ideally, this should be forward compatible.

The second point was indeed a problem.

I added apply = toString to the option definitions to avoid these problems.

Comment thread modules/misc/xdg-user-dirs.nix Outdated
type = with types; attrsOf (either str path);
apply = builtins.mapAttrs (_: toString);
default = { };
example = { XDG_MISC_DIR = "$HOME/Misc"; };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's fix this while you are at it already

Suggested change
example = { XDG_MISC_DIR = "$HOME/Misc"; };
example = literalExpression ''
{
XDG_MISC_DIR = "$HOME/Misc";
}
'';

Comment thread modules/misc/xdg-user-dirs.nix
@ilkecan ilkecan force-pushed the xdg-user-dirs-improvements branch from 76c2e63 to ea52bff Compare February 25, 2022 15:57
Comment thread modules/misc/xdg-user-dirs.nix Outdated
Comment on lines +92 to +93
type = with types; attrsOf (either str path);
apply = builtins.mapAttrs (_: toString);
Copy link
Copy Markdown
Contributor

@polykernel polykernel Feb 28, 2022

Choose a reason for hiding this comment

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

Suggested change
type = with types; attrsOf (either str path);
apply = builtins.mapAttrs (_: toString);
type = with types; attrsOf (coercedTo path toString str);

minor nitpick, I think coercedTo is better suited here as only type coercion is happening, rather than transformation of the option value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I wasn't aware of the coercedTo type.

@ilkecan ilkecan force-pushed the xdg-user-dirs-improvements branch from ea52bff to 0d92dc3 Compare February 28, 2022 11:24
Changed option types to `either str path` to allow using path values.

The related session variable is defined for the default and the extra
user directories now.
@ilkecan ilkecan force-pushed the xdg-user-dirs-improvements branch from 0d92dc3 to f21803d Compare March 24, 2022 23:47
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Thanks again!

@berbiche berbiche merged commit 3549f5d into nix-community:master Apr 5, 2022
@ilkecan ilkecan deleted the xdg-user-dirs-improvements branch April 5, 2022 13:43
jficz pushed a commit to jficz/home-manager that referenced this pull request Apr 7, 2022
…#2757)

Changed option types to `either str path` to allow using path values.

The related session variable is defined for the default and the extra
user directories now.
@teto teto mentioned this pull request Aug 22, 2022
7 tasks
teto pushed a commit to teto/home-manager that referenced this pull request Aug 22, 2022
…#2757)

Changed option types to `either str path` to allow using path values.

The related session variable is defined for the default and the extra
user directories now.
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
…#2757)

Changed option types to `either str path` to allow using path values.

The related session variable is defined for the default and the extra
user directories now.
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.

More consistent XDG support

4 participants