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

easyeffects: add option to import presets #6267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hauskens
Copy link

@hauskens hauskens commented Jan 4, 2025

Description

  • added options to import presets
  • added tests
  • added hausken as maintainer

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.
    (tested with nix-shell --pure tests -A run.easyeffects-service and nix-shell --pure tests -A run.easyeffects-example-preset)

  • 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.

Maintainer CC

@fufexan

@hauskens hauskens force-pushed the easyeffects-presets branch from ef51b15 to 4463220 Compare January 4, 2025 19:21
Comment on lines 90 to 91
extraInputPresets = presetOptionType;
extraOutputPresets = presetOptionType;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could simply be

Suggested change
extraInputPresets = presetOptionType;
extraOutputPresets = presetOptionType;
extraPresets = presetOptionType;

where the "input" or "output" field inside the preset determine whether it is for input or output?

Copy link
Member

Choose a reason for hiding this comment

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

That is, the type of this option would be something like attrsOf presetType where presetType would be the regular JSON type with the additional requirement that it is an object containing either an "input" or "output" field. In practice I think this would look something like

presetType = let baseType = attrsOf jsonFormat.type; in baseType // {
  check = v: baseType.check v && elem (attrNames v) == [ ["input"] ["output"] ];
  description = "EasyEffects input or output JSON preset";
}

Completely untested so I'm not certain it will work very well.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the input, i really appreciate it!
I agree, it makes much more sense to have it in one attribute set, so i've modified it based on your suggestions.
I'm fairly inexperienced with programming in nix, so this feedback helped a lot.
Let me know if there is anything that comes to mind

added options to import presets
added tests
added hausken as maintainer
@hauskens hauskens force-pushed the easyeffects-presets branch from 4463220 to f50c5ca Compare January 5, 2025 17:04
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