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

feat: fully configurable plz menu #110

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x61nas
Copy link
Contributor

@0x61nas 0x61nas commented Aug 27, 2023

Description

The goal is to provide full power to our users and make them able to configure and create their custom plz menus easily

in order to improve the project and take it to the next level, this PR introduces a bunch of breaking changes to both the configuration system and the command-line interface.

  • Change the plz menu system to read the menu entries from the config file
    • Each entry contains its display string that will displayed in the menu and its operation/args
    • The operation is just a hash map that contains the operation key and its value and the rest of the arguments
    • Each operation has a specific set of pre-defined arguments that can be used to configure the operation handler and change its default behavior.
    • The operation handler will ignore any argument that doesn't know about
    • However, all the argument values will be passed to the template engine to be parsed before they are passed to the operation handler, which means that if there's any template error in any of the arguments the program will exit even if the argument that have the error doesn't needed by the operation handler
    • The currently available operations is:
      • fetch | url: fetch any page from the web(mainly) by HTTP
      • run | command: run command/s
      • file: just display the contents of any local file in the pager
  • Remove the following CLI options:
    • --cheat-sh-url
    • --eg-url
    • --man-cmd
    • --cheat-url
  • Introduce the following CLI options:
    • -s | --selected-position: used to set the default selected position, the posable values are:
      • start: The first item in the menu
      • center: The middle item in the menu (in case of an even number of items, the first item in the second half)
      • end: The last item in the menu

Motivation and Context

The current implementation for the plz menu is nearly limited (there are some configuration options but not many). and we need to give the user full control. This change addresses this demand by introducing new features, refining existing systems, and optimizing the menu's behavior.

How Has This Been Tested?

  • Just by cargo test --lib
  • And try the tool with different possible configurations and ensure that they work as expected

Screensthot

asciicast

The config file in this example:

check_version = true
check_help = true
check = [["-v", "-V", "--version", "version"], ["-h", "--help", "help", "-H"]]
man_command = "man"
pager_command = "less -R"

[plz_menu]
selected_pos = "Center"

[[plz_menu.entries]]
display_msg = "Show man page"

[plz_menu.entries.operation]
run = "man {cmd}"

[[plz_menu.entries]]
display_msg = "Show cheat.sh page"

[plz_menu.entries.operation]
user-agent = "fetch"
fetch = "https://cheat.sh/{cmd}{?/{subcommand}}{? {args}}"

[[plz_menu.entries]]
display_msg = "Show eg page"

[plz_menu.entries.operation]
fetch = "https://raw.githubusercontent.com/srsudar/eg/master/eg/examples/{cmd}.md"

[[plz_menu.entries]]
display_msg = "Show cheatsheets page"

[plz_menu.entries.operation]
fetch = "https://raw.githubusercontent.com/cheat/cheatsheets/master/{cmd}"

[[plz_menu.entries]]
display_msg = "Show info page"

[plz_menu.entries.operation]
file = "~/info-pages/{cmd}.md"

[[plz_menu.entries]]
display_msg = "The TLDR page"

[plz_menu.entries.operation]
run = "tldr {cmd}"
use-pager = "true"

[[plz_menu.entries]]
display_msg = "The navi fuzzy search page"

[plz_menu.entries.operation]
run = "navi --query '{cmd}{? {subcommand}}'"

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Refactor
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Update the readme file
  • Update the config/halp.toml file

@0x61nas 0x61nas marked this pull request as ready for review August 27, 2023 07:08
@0x61nas 0x61nas requested a review from orhun as a code owner August 27, 2023 07:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2023

Codecov Report

Patch coverage is 68.53% of modified lines.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files Changed Coverage
src/error.rs ø
src/helper/docs/handlers/command.rs 0.00%
src/helper/docs/handlers/file.rs 0.00%
src/helper/docs/mod.rs 0.00%
src/lib.rs 50.00%
src/helper/docs/handlers/fetch.rs 83.12%
src/helper/docs/template.rs 96.37%
src/cli.rs 100.00%
src/config/mod.rs 100.00%
src/config/plz_menu.rs 100.00%
... and 2 more

📢 Thoughts on this report? Let us know!.

@orhun
Copy link
Owner

orhun commented Sep 1, 2023

Hey, thanks a lot for the PR! I will probably get to this during the weekend.

There seems to be some conflicts, can you check?

@0x61nas
Copy link
Contributor Author

0x61nas commented Sep 1, 2023

I will probably get to this during the weekend.

take your time

There seems to be some conflicts, can you check?

fixed 👍🏻

@orhun
Copy link
Owner

orhun commented Sep 15, 2023

Hey, sorry for taking too long but just got some time to review this. I'm actually in an airport currently and trying to wrap my head around these changes 🐻

So first of all, this is quite a large PR which begs the following questions:

  1. Can we handle this in a simpler manner?

I just checked the halp.toml and the structure was not really intuitive for me:

[plz_menu]
selected_pos = "Center"

[[plz_menu.entries]]
display_msg = "Show man page"

[plz_menu.entries.operation]
run = "man {cmd}"

[[plz_menu.entries]]
display_msg = "Show cheat.sh page"

[plz_menu.entries.operation]
user-agent = "fetch"
fetch = "https://cheat.sh/{cmd}{?/{subcommand}}{? {args}}"

[[plz_menu.entries]]
display_msg = "Show eg page"

[plz_menu.entries.operation]
fetch = "https://raw.githubusercontent.com/srsudar/eg/master/eg/examples/{cmd}.md"

[[plz_menu.entries]]
display_msg = "Show cheatsheets page"

[plz_menu.entries.operation]
fetch = "https://raw.githubusercontent.com/cheat/cheatsheets/master/{cmd}"

Defining arrays in TOML are a bit weird already and it makes it harder to read when we have granual entries like this, Maybe we should consider merging these under one struct like [[entry]].

Another option is to document these well in README.md and include some examples. But we should definitely look into simplifying this.

  1. Can this be handled in separate PRs?

I saw that there are quite a bit of commits and it makes it harder to review everything. Maybe consider squashing some of the commits and/or creating separate PRs for finishing this.

  1. Should we release a version before this?

There was some discussion about this but I guess I assumed that this will be a simple change which was lowkey wrong. So, should we go ahead with a release before this breaking change?


Overall thanks for creating this! We will definitely need to tweak some parts but in the end I really want to have this feature in halp.

@0x61nas
Copy link
Contributor Author

0x61nas commented Sep 15, 2023

  1. Can we handle this in a simpler manner?

I also didn't want to be in this form initially, as I mentioned in the related issue (#96) I thought that it probably could be done in a just two-dimensional array.
but as we know the ideas are a thing and implementing these ideas is a different thing, especially when we depend on some third-party code

I found that the 2D array way would convert the code into a complete mess and it'll make it hard to maintain so I dropped that option from my list

I've tried to represent the operation with an enum, we've got a limited number of operations and we know that every operation takes a limited amount of parameters. so this makes perfect sense to use an enum for, and obviously, we won't use the enums in Rust without taking its full power, and this will make our life a bit easier when we need to read these values from the other places in our code so we won't need to check every time we need them. but the short long story the toml deserializer wasn't quite happy with that so I had to take another approach

I just checked the halp.toml and the structure was not really intuitive for me:

You're not alone :)

Maybe we should consider merging these under one struct like [[entry]].

I didn't get what you mean, can you provide some more information?

Another option is to document these well in README.md and include some examples. But we should definitely look into simplifying this.

  1. Can this be handled in separate PRs?

I have planned to do this already but I was waiting for this PR to get merged or at least get reviewed, so I can consider the changes when I write.

I saw that there are quite a bit of commits and it makes it harder to review everything.

Sorry about that, am still learning

Maybe consider squashing some of the commits

I'll do that

  1. Should we release a version before this?

I guess this's the right move for now, but consider updating the halp plz --help command output in the README.md cause I forgot to do that in #56

https://github.com/orhun/halp/blob/main/README.md?plain=1#L236-L243

Options:
  -m, --man-cmd <MAN_CMD>   Sets the manual page command to run
      --cheat-sh-url <URL>  Use a custom URL for cheat.sh [env: CHEAT_SH_URL=]
+   --eg-url <URL>  Use a custom provider URL for eg pages. [env: EG_PAGES_URL=]
+   --cheat-url <URL>  Use a custom provider URL for cheat sheets. [env: ECHEATSHEETS_URL=]
  -p, --pager <PAGER>       Sets the pager to use
      --no-pager            Disables the pager
  -h, --help                Print help

@0x61nas 0x61nas force-pushed the feat/configrable-plz-menu branch from da8bb01 to ff5e89f Compare September 16, 2023 13:19
BREAKING CHANGE: this PR introduces a bunch of breaking changes to both the configuration system and the command-line interface.

- Change the `plz` menu system to read the menu entries from the config file
   - Each entry contains its display string that will displayed in the menu and its operation/args
   - The `operation` is just a hash map that contains the operation key and its value and the rest of the arguments
   - Each operation has a specific set of pre-defined arguments that can be used to configure the `operation handler` and change its default behavior.
   -  The operation handler will ignore any argument that doesn't know about
   - However, all the argument values will be passed to the template engine to be parsed before they are passed to the operation handler, which means that if there's any `template error` in any of the arguments the program will exit even if the argument that have the error doesn't needed by the operation handler
   - The currently available operations is:
      -  `fetch` | `url`: fetch any page from the web(mainly) by HTTP
      - `run` | `command`: run command/s
      - `file`: just display the contents of any local file in the pager
- Remove the following CLI options:
   - `--cheat-sh-url`
   - `--eg-url`
   - `--man-cmd`
   - `--cheat-url`
 - Introduce the following CLI options:
    - `-s` | `--selected-position`: used to set the default selected position, the posable values are:
       - `start`:  The first item in the menu
       - `center`: The middle item in the menu (in case of an even number of items, the first item in the second half)
       - `end`:    The last item in the menu

COMMITS LIST:

da8bb01 fix: un-use the wildcard pattern and use `as` instead (2 weeks ago)
0fdc42e chore(git): merge main and resolve confilcts (2 weeks ago)
9fd106d docs(readme): update the outdated examples (3 weeks ago)
446efff chore(config): just update the config file (3 weeks ago)
a418fbd fix(test): fix the cli::update_config test and fi doc (3 weeks ago)
0187928 feat(file): create the file handler (3 weeks ago)
a08101f feat(cli): add the --selected-position option to plz subcommand (3 weeks ago)
6d07d80 refactor: refactor and and lint and re-format the whole proj (3 weeks ago)
4dd0702 feat(command): create the command operation handler (3 weeks ago)
3eb71a6 refactor(plz): improve the operation handleing system (3 weeks ago)
3bb10a6 refactor: remove all the deprecate code and re-format the code (3 weeks ago)
9bfc239 revert(idea): remove the `.idea` directory (3 weeks ago)
1babee0 feat(plz): use the new system (3 weeks ago)
f4d845e feat(cmd_parse): take a refrence to the hashmap (3 weeks ago)
0ed22dd feat(fetch): create the fetch handler and stabilize the `Handler` interface (3 weeks ago)
1646c14 feat(parser): cover more cases for the command parser (3 weeks ago)
5b054fb feat(parser): create the command parser (3 weeks ago)
399dfee feat(parser): create the template parser (3 weeks ago)
ad409ab feat(plz): make the defualt selected item configrable (4 weeks ago)
218316a feat(plz): improve the cheat.sh url template (4 weeks ago)
08e7867 feat(plz): create the handler trait (4 weeks ago)
bba0696 feat(plz): read the plz menu from the config file (4 weeks ago)
@0x61nas 0x61nas force-pushed the feat/configrable-plz-menu branch from ff5e89f to 148480b Compare September 16, 2023 14:06
@orhun
Copy link
Owner

orhun commented Mar 1, 2024

💀

@0x61nas
Copy link
Contributor Author

0x61nas commented Mar 4, 2024

died pr, I was too lazy to work on the suggestions, sorry about that. but I'll try to fix it sometime soon, I hope.

I was using it on my machine all this time btw. and I didn't open the config file, I didn't need to do that beside the initial time 7 months ago. so you can see why I did get lazy.

@0x61nas
Copy link
Contributor Author

0x61nas commented Mar 4, 2024

But if someone else can fix the (de)serialization mess, I'll not say no. Am not that TOML enthusiast anyway.

@orhun
Copy link
Owner

orhun commented Mar 4, 2024

Yeah, I need to read through everything again. I don't quite remember what was the status with this.

@orhun orhun marked this pull request as draft June 20, 2024 15:41
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.

make the plz menu configurable via the config file
3 participants