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

Refactor commands #297

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Refactor commands #297

wants to merge 3 commits into from

Conversation

deemp
Copy link
Contributor

@deemp deemp commented Jan 13, 2024

  • Move parts of the commands module to separate files

This was referenced Jan 13, 2024
@deemp deemp force-pushed the refactor-commands branch 2 times, most recently from d035c0d to 945a241 Compare January 14, 2024 00:52
@zimbatm
Copy link
Member

zimbatm commented Jan 14, 2024

thanks, LGTM but just needs a rebase

@deemp
Copy link
Contributor Author

deemp commented Jan 14, 2024

Rebased

@@ -121,7 +121,7 @@ let

# TODO: handle opt.relatedPackages. What is it for?
optToMd = opt:
let heading = lib.showOption opt.loc; in
let heading = (lib.showOption (filter isString opt.loc)) + (concatStrings (filter (x: !(isString x)) opt.loc)); in
Copy link
Member

Choose a reason for hiding this comment

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

nit: the surrounding parenthesis is not needed

Copy link
Contributor Author

@deemp deemp Jan 18, 2024

Choose a reason for hiding this comment

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

Fixed that

Comment on lines +46 to +48
```console
(package or string convertible to it) or (list with two elements of types: [ string (package or string convertible to it) ]) or (flatOptions)
```
Copy link
Member

Choose a reason for hiding this comment

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

For flake-parts users, this mix of types would make it hard to merge commands coming from different places. I would rather keep either the list, or the attrset of commands, but not both.

Let's keep things simple.

Copy link
Contributor Author

@deemp deemp Jan 16, 2024

Choose a reason for hiding this comment

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

flake-parts

I had no problems with flake-parts. See repro. May require nix flake update.

type of commands

I couldn't invent a new option name so I made commands either a list or an attrset.

I prefer attrsets with attrNames as command group names.

I think it should be the first-class feature and should be documented and type checked.

So, I see these approaches:

  1. Allow commands only as an attrset
  2. Create a new option for commands as an attrset, leave commands as a list
    • What should be the name?
  3. Allow commands to be either a list or an attrset, do something about merging lists and attrsets for flake-parts users.
    • What in particular can make it hard to merge?
    • I need a concrete example of a devshell setup with flake-parts
    • I find it nice to have a simpler representation that can be used to explain the nested options.
  4. Provide just a function that converts a commands attrset to a list
    • I don't like this idea because then commands attrset won't be the first-class feature and can't naturally go into docs.

@deemp
Copy link
Contributor Author

deemp commented Jan 24, 2024

@zimbatm, could you please review this PR and #290?

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