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

New syntax for customize mode #1709

Merged
merged 19 commits into from
Nov 16, 2023
Merged

Conversation

yannham
Copy link
Member

@yannham yannham commented Nov 6, 2023

Implement ideas described in #1408 to improve the customize mode and avoid clashes between arguments and predefined flags such as --override.

Overview

We now use a <field path>=<nickel expression> syntax for the customize mode:

$ nickel export config.ncl -- input.bar.baz=1 'input.foo.bar="Hello, world!"' --override output.value=false

Having positional assignments also leaves the ability to add subcommands. This PR adds two of them, list and show (plus help automatically generated by clap):

$ nickel eval examples/config-gcc/config-gcc.ncl -- list
Input fields:
- path_libc: <Path,SharedObjectFile>

Overridable fields (require `--override`):
- flags: <Array GccFlag>
- optimization_level: <OptLevel>
$ nickel eval examples/config-gcc/config-gcc.ncl -- show path_libc
• contract: Path,SharedObjectFile
• default: "/lib/x86_64-linux-gnu/libc.so"
• documentation: Path to libc.

Refactoring

As part of this PR, a few structural changes have been performed:

  • The part of the cli::customize module which handles extracting an interface from a term has been separated into its own submodule cli::customize::interface. It might come in handy if we want to make this notion of interface useful beyond the customize mode (e.g. for generating a man page from a configuration).
  • The parsing of QueryPath has been moved entirely in the parser (some post processing was still done in program), to make it consistent with the new code introduced to parse assignments on the command line
  • QueryPath is renamed FieldPath, because it's now useful beyond queries, and is just a wrapper around Vec<LocIdent> which represents a structured field path.

Bikeshedding

There are different possibilities for the subcommands introduced, and I'm not entirely set:

  • nickel eval examples/config-gcc/config-gcc.ncl -- show path_libc is currently almost equivalent to nickel query examples/config-gcc/config-gcc.ncl --path path_libc.
    • Should we really have two different subcommands, or just mention in the help message to use nickel query instead of introducing show? It's more consistent to avoid duplicate, but on the other hand, one could argue the users would be exploring possibilities directly from the customize mode, and that changing the main subcommand is a non-obvious context switch
    • If we keep show, should we name it query as well to be consistent, or keep show to avoid confusion between the two variants of query?
  • Similarly, -- list is not very far from nickel query without specifying a field path. Though it is not exactly the case, because -- list makes a distinction between input fields and overridable fields, which is important for the UX of the customize mode.

Future work

  • Make the output of -- list prettier

@yannham yannham marked this pull request as draft November 6, 2023 18:26
@yannham yannham force-pushed the feature/positional-customize-mode branch from f116ca3 to e9c48b6 Compare November 6, 2023 18:27
@github-actions github-actions bot temporarily deployed to pull request November 6, 2023 18:32 Inactive
@yannham yannham marked this pull request as ready for review November 7, 2023 08:14
@github-actions github-actions bot temporarily deployed to pull request November 7, 2023 08:17 Inactive
@jneem
Copy link
Member

jneem commented Nov 9, 2023

I haven't looked at the code yet, but for the bikeshedding I think I prefer having a consistent hierarchy of commands over grouping commands that we think might be used together. I don't feel like going from nickel eval to nickel query involves much of a context switch, and I find it easy to remember that nickel query is for querying/showing/listing the configuration, while nickel eval is for evaluating it.

@yannham
Copy link
Member Author

yannham commented Nov 10, 2023

@jneem so, you propose to get rid of the show subcommand, which is subsumed by query. What about -- list, which is somehow similar to nickel query without a path, but shows a different output (shows the contracts of fields and if they are inputs or overridable)? Should we just keep that, in your opinion?

@yannham yannham added this to the Next minor (1.3) milestone Nov 12, 2023
@jneem
Copy link
Member

jneem commented Nov 13, 2023

Yeah, good question. I guess it feels like list is more specialized to the customize mode, so maybe it belongs there rather than query? But on the other hand it is kind of a special output mode for querying the configuration.

Maybe nickel query --list-overrides is reasonable? The --list-overrides flag would change the output format of query, so that rather than showing metadata of the queried element it would list its overridable fields. And it could be compatible with --path, in case there are overridable fields that are not at top-level...

Edit: but also, I don't think we need to block the release on bikeshedding this. It's experimental anyway...

cli/src/customize/interface.rs Show resolved Hide resolved
cli/src/customize/interface.rs Outdated Show resolved Hide resolved
cli/src/customize/mod.rs Show resolved Hide resolved
cli/src/error.rs Outdated Show resolved Hide resolved
@yannham yannham force-pushed the feature/positional-customize-mode branch from 1174102 to ce6e616 Compare November 14, 2023 22:03
@github-actions github-actions bot temporarily deployed to pull request November 14, 2023 22:06 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 15, 2023 14:54 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 15, 2023 17:59 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 16, 2023 10:14 Inactive
@yannham yannham added this pull request to the merge queue Nov 16, 2023
Merged via the queue into master with commit f186e0e Nov 16, 2023
5 checks passed
@yannham yannham deleted the feature/positional-customize-mode branch November 16, 2023 10:30
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