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

cli, config: more granular pagination settings #2927

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jrobsonchase
Copy link
Collaborator

@jrobsonchase jrobsonchase commented Feb 3, 2024

  • Adds "always" as a pagination option
  • Adds a new cli arg, --pagination that takes a pagination string (always, never, or auto).
  • Adds a new type, Pagination, which may be either one of the pagination options, or an arbitrarily nested mapping of subcommands. When nested, "default" is the setting for the current level, and may be overridden by deeper subcommands. A setting of "auto" will "fall through" to the previous level.

"help" is a little odd, since it doesn't get the context of the subcommand being attempted. All help messages use the configuration for the top-level "help" subcommand.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Resolves #2926

* Adds "always" as a pagination option
* Adds a new cli arg, `--pagination` that takes a pagination string (always, never, or auto).
* Adds a new type, `Pagination`, which may be either one of the pagination
  options, or an arbitrarily nested mapping of subcommands. When nested,
  "default" is the setting for the current level, and may be overridden by deeper
  subcommands. A setting of "auto" will "fall through" to the previous level.

"help" is a little odd, since it doesn't get the context of the subcommand
being attempted. All help messages use the configuration for the top-level
"help" subcommand.

Still needs:
- [ ] testing
- [ ] documentation
next = args.subcommand();
Some(sub)
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: perhaps, iter::successors(..).map(..) can be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, neat - I think I knew about this at one time, but forgot about it.

lol, and the source even has a comment alluding to this exact implementation of it 😂

pub fn successors<T, F>(first: Option<T>, succ: F) -> Successors<T, F>
where
    F: FnMut(&T) -> Option<T>,
{
    // If this function returned `impl Iterator<Item=T>`
    // it could be based on `from_fn` and not need a dedicated type.
    // However having a named `Successors<T, F>` type allows it to be `Clone` when `T` and `F` are.
    Successors { next: first, succ }
}

I'll switch to that and likely forget about it and get to rediscover it all over again in the future 👍

) => ControlFlow::Continue((out, p)),
Some(p) => ControlFlow::Continue((p.default, p)),
},
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this is interesting, I think it's better to use a flat map of (full_command_name, paginate). Subcommands tree is just a namespace. Child command doesn't inherit parent property in general.

I'm not sure whether the config table should be pager-specific or more general [commands.<name>], but something like this might be useful:

[commands."branch list"]
paginate = ..
pager = ..

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.

FR: selectively enable/disable pagination
2 participants