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

Improve NixOS module #1059

Closed

Conversation

erikarvstedt
Copy link
Contributor

No description provided.

@erikarvstedt erikarvstedt force-pushed the improve-nixos-module branch from c5fbb96 to 9afabd0 Compare March 4, 2020 23:38
@erikarvstedt
Copy link
Contributor Author

Rebased.

@lovesegfault
Copy link
Contributor

Pinging @rycee for review

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

I can't comment on the functionality of the code, but I have a few suggestions I'd like to make.

README.md Outdated
Comment on lines 9 to 10
For a more systematic documentation of Home Manager and all available
options please see the [Home Manager manual][manual].
Copy link
Member

Choose a reason for hiding this comment

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

This wording feels awkward, to me. If it needs to be changed at all, maybe something more along the lines of:

Suggested change
For a more systematic documentation of Home Manager and all available
options please see the [Home Manager manual][manual].
For a more systematic overview of Home Manager and its available
options, please see the [Home Manager manual][manual].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Editing help from native speakers is highly welcome, thank you!

<note>
<para>
By default, Home Manager modules use a private <literal>pkgs</literal> instance
that's configured via the <option>nixpkgs</option> options.
Copy link
Member

Choose a reason for hiding this comment

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

Pretty nitpick-y, but I hold to the belief that documentation should be more formal (and thus refrain from using contractions and other colloquialisms).

Suggested change
that's configured via the <option>nixpkgs</option> options.
that is configured via the <option>nixpkgs</option> options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


<para>
To easily set Home Manager options for a specific user, use the following recipe.
This is useful when a user config is composed through multiple NixOS modules.
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary, but sounds better, IMHO.

Suggested change
This is useful when a user config is composed through multiple NixOS modules.
This is especially useful when a user's configuration is composed through
multiple NixOS modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

modules/modules.nix Show resolved Hide resolved
nixos/default.nix Outdated Show resolved Hide resolved
modules/misc/news.nix Show resolved Hide resolved
@erikarvstedt erikarvstedt force-pushed the improve-nixos-module branch from 9afabd0 to 248dbae Compare March 6, 2020 19:23
@@ -45,6 +46,11 @@ in {
<option>users.users.‹name?›.packages</option> option.
'';

useGlobalPkgs = mkEnableOption ''
using <literal>pkgs</literal> from the NixOS modules in Home Manager.
Copy link
Member

Choose a reason for hiding this comment

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

Nothing important but perhaps

Suggested change
using <literal>pkgs</literal> from the NixOS modules in Home Manager.
using the system configuration's <literal>pkgs</literal> argument in Home Manager.

or something would be more clear?

@@ -5,6 +5,9 @@

# Whether to enable module type checking.
, check ? true

# If disabled, the pkgs passed to this function are used instead
Copy link
Member

@rycee rycee Mar 7, 2020

Choose a reason for hiding this comment

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

Suggested change
# If disabled, the pkgs passed to this function are used instead
# If disabled, the pkgs attribute passed to this function is used instead.

_module.args.pkgs = lib.mkDefault pkgs;
_module.check = check;
lib = lib.hm;
} // optionalAttrs (useNixpkgsModule) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} // optionalAttrs (useNixpkgsModule) {
} // optionalAttrs useNixpkgsModule {

rycee pushed a commit that referenced this pull request Mar 7, 2020
The main manual page is highly relevant and should be easily
discoverable from the README.

PR #1059
Fixes #1048
@rycee
Copy link
Member

rycee commented Mar 7, 2020

I cherry picked the README: add link to main manual page commit to master in c7b4378.

<note>
<para>
By default, Home Manager modules use a private <literal>pkgs</literal> instance
that is configured via the <option>nixpkgs</option> options.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
that is configured via the <option>nixpkgs</option> options.
that is configured via the <option>home-manager.users.&lt;name?&gt;.nixpkgs</option> options.

Comment on lines 233 to 234
To disable these options and to use the global <literal>pkgs</literal> from
the NixOS modules, set
Copy link
Member

@rycee rycee Mar 7, 2020

Choose a reason for hiding this comment

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

Suggested change
To disable these options and to use the global <literal>pkgs</literal> from
the NixOS modules, set
To instead use the global <literal>pkgs</literal> that is configured via
the <option>nixpkgs</option> options, set

doc/installation.xml Outdated Show resolved Hide resolved
@erikarvstedt erikarvstedt force-pushed the improve-nixos-module branch from 248dbae to 45b997a Compare March 7, 2020 13:02
</para>
</note>

<para>
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest removing this commit since it is orthogonal to the general PR and I think does not fit in the installation section. Maybe could go in a "tips and tricks" section (in a separate PR) or even on the wiki?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it. Where would you add that tips and tricks section?

Copy link
Member

Choose a reason for hiding this comment

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

I guess there should be a new chapter called something like "NixOS Integration", with a section "Tips and Tricks" where this would go into a subsection.

I think there you could also have more space to write text surrounding this use pattern since it is dealing with more advanced concepts such as defining your own options and using a function like mkAliasDefinitions.

@erikarvstedt
Copy link
Contributor Author

Here's a rebased version of this PR.

@erikarvstedt erikarvstedt force-pushed the improve-nixos-module branch from 45b997a to f209fbf Compare March 7, 2020 13:10
rycee pushed a commit that referenced this pull request Mar 7, 2020
@rycee
Copy link
Member

rycee commented Mar 7, 2020

Thanks! Added to master in efbe138.

@rycee rycee closed this Mar 7, 2020
jorsn pushed a commit to jorsn/home-manager that referenced this pull request Apr 25, 2020
The main manual page is highly relevant and should be easily
discoverable from the README.

PR nix-community#1059
Fixes nix-community#1048
jorsn pushed a commit to jorsn/home-manager that referenced this pull request Apr 25, 2020
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.

4 participants