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

ncmpcpp: add module #1457

Closed
wants to merge 5 commits into from
Closed

Conversation

olmokramer
Copy link
Contributor

Description

Adds a module for https://github.com/ncmpcpp/ncmpcpp. All settings are configured through a single settings attrset, except for mpd_music_dir, which has its own dedicated option. If it is set to null (the default) its value is taken from services.mpd.musicDirectory and services.mpd.enable is required to be true.

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.

@olmokramer olmokramer requested a review from rycee as a code owner August 26, 2020 20:26
@olmokramer olmokramer force-pushed the ncmpcpp branch 4 times, most recently from a5dc6f4 to 015f640 Compare August 26, 2020 20:41
example = "pkgs.ncmpcpp.override { ... }";
};

mpdMusicDir = mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could get rid of this option and place it under settings if settings were a freeform module, but that breaks the tests with an error:

Module `:anon-1:anon-1' has an unsupported attribute `freeformType'.
This is caused by introducing a top-level `config' or `options'
attribute. Add configuration attributes immediately on the top level
instead, or move all of them (namely: freeformType) into the
explicit `config' attribute.

The Freeform modules PR has only been merged 12 days ago, though, so it may not be a great idea to start relying on it already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I'll have to check out 🙂

modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
@olmokramer
Copy link
Contributor Author

Thanks for the review! I have implemented all your suggestions except for the bindings type, and added a warning if both programs.ncmpcpp.mpdMusicDir and programs.ncmpcpp.settings.mpd_music_dir are set.

modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
@olmokramer
Copy link
Contributor Author

Thanks again! I've applied your changes and added a with lib; at the top, since all modules seem to do that and you seem to be expecting that as well :)

modules/programs/ncmpcpp.nix Outdated Show resolved Hide resolved
@olmokramer
Copy link
Contributor Author

Ok I've changed the default value of programs.ncmpcpp.mpdMusicDir to only use the MPD configuration on Linux platforms. Still not quite sure what you meant with your last comment.

I've also "resolved" the comments that I have addressed for my own convenience, but feel free to "unresolve" them if you think they deserve more attention :)

@olmokramer
Copy link
Contributor Author

I've moved the test of the MPD configuration to a new directory and only run it on Linux now.

Also rebuilt the documentation one last time, and I noticed that the default for programs.ncmpcpp.mpdMusicDir is not rendered literally, i.e. the newlines and indentation are removed. Is there some alternative to literalExample that is supposed to be used for the default value? Or is the default value text never rendered literally?

rycee pushed a commit that referenced this pull request Sep 1, 2020
@rycee
Copy link
Member

rycee commented Sep 1, 2020

Thanks! Rebased to master in 4b702bf.

@rycee rycee closed this Sep 1, 2020
aakropotkin pushed a commit to aakropotkin/home-manager that referenced this pull request Sep 9, 2020
malte-v pushed a commit to malte-v/home-manager that referenced this pull request Feb 24, 2021
@olmokramer olmokramer deleted the ncmpcpp branch January 4, 2023 22:36
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.

2 participants