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

Make it clear the "details" of command are available #3890

Merged
merged 3 commits into from
Dec 25, 2023

Conversation

byteninjaa0
Copy link
Contributor

@byteninjaa0 byteninjaa0 commented Sep 26, 2023

Your checklist for this pull request

  • [ yes] I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description
now when we run the w? command it will show that w?? option is also available

...

Test plan

...

Closing issues

Closes #3879

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

Hi and thanks for the contribution! Some comments on the PR:

  • I don't think this is exactly what @XVilka meant in his bug. We would like something that would be generic and not specific to the w command. There are many other commands that have ?? available and we want to show a similar help message for all of them. That's why we have rzshell
  • these cmd_descs.c/cmd_descs.h files, as you can read at the head of the files, are automatically generated from the .yaml files you can find at https://github.com/rizinorg/rizin/tree/dev/librz/core/cmd_descs . You should modify the .yaml files and re-build the project (this will update the cmd_descs.c/cmd_descs.h files).
  • However, since this change should be done for all commands that have some "details" available, I believe this change should be done somewhere in cmd_api.c

@byteninjaa0
Copy link
Contributor Author

show i do the same for other commands too?

@ret2libc
Copy link
Member

show i do the same for other commands too?

Yes but in a general way. You should not modify the summary of each command manually.

librz/core/cmd/cmd_api.c Outdated Show resolved Hide resolved
@wargio
Copy link
Member

wargio commented Sep 27, 2023

please install pyaml and clang-format, then rebuild and run ./sys/clang-format.py before pushing.

@byteninjaa0
Copy link
Contributor Author

when i am run w? it is showing the message four times .

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

image

I don't like how this looks.
IMHO something like (%s?? for more details) should be appended to the summary of the top command

image

@ret2libc
Copy link
Member

ret2libc commented Oct 9, 2023

Please show a screenshot of how it looks like now!

@byteninjaa0
Copy link
Contributor Author

image

@ret2libc
Copy link
Member

I think it is not working as expected yet. Like, wv? does not show the message. Or we? says to use we?? but actually only wex has details...

Now, I think given that when a command is a leaf (there are no subcommands) the details are automatically displayed (try wex?, for example), I think we can just show the message <x>?? for more details near the summary of the subcommand with the same name as the group name.

For example:

  • w is both a group (w? displays the sub-commands) and a command (w?? shows the detail of the w command). In this case, it would make sense to have in the w? screen the message (w?? for more details) near the w command.

  • we is the name of a group (we? shows the sub-commands), but there is no sub-command with the same name (there is no we command), thus nothing should be shown.

  • wex is a leaf command, thus nothing should be shown. When you do wex? it already shows the details

  • wv is both a group (wv? displays the sub-commands) and a command (wv?? shows the details of the wvcommand). In this case it would make sense to have in thewv?screen the message(wv?? for more details)near thewv` command.

I hope it make sense.

@byteninjaa0
Copy link
Contributor Author

thank you for the detailed explanation, making a commit soon!

@wargio
Copy link
Member

wargio commented Oct 11, 2023

what if we do something like tree view where if there are more subcommands to it we have a + instead of | at the beginning of the line?
@ret2libc @XVilka

@ret2libc
Copy link
Member

what if we do something like tree view where if there are more subcommands to it we have a + instead of | at the beginning of the line? @ret2libc @XVilka

Interesting ideas... We'd need to see how it looks like, but in theory I don't mind it. It would make things clearer immediately. However, it's a slightly different problem. The +/| says whether a command has some subcommands or not. The (use <cmd>?? for more details) tells you whether you should use <cmd>?? to get more info about a subcommand with the same name as the name of a group (w group and w command, for example).

@wargio
Copy link
Member

wargio commented Oct 11, 2023

The (use <cmd>?? for more details) tells you whether you should use <cmd>?? to get more info about a subcommand with the same name as the name of a group (w group and w command, for example).

The issue i see is that this might make the command line long.

@byteninjaa0
Copy link
Contributor Author

Screenshot 2023-10-22 115942
Screenshot 2023-10-22 120009
Screenshot 2023-10-22 120033
Screenshot 2023-10-22 120056

librz/core/cmd/cmd_api.c Outdated Show resolved Hide resolved
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

run clang format (requires clang format 16 and then run ./sys/clang-format.py)

@byteninjaa0
Copy link
Contributor Author

same test is failed again i am seeing into the issue i think we have to make our unit test flexible for the edited summary of subcommand

@byteninjaa0
Copy link
Contributor Author

is there anything else left in this?

@XVilka
Copy link
Member

XVilka commented Dec 25, 2023

is there anything else left in this?

green CI

@XVilka XVilka merged commit 8c275bf into rizinorg:dev Dec 25, 2023
44 checks passed
@byteninjaa0 byteninjaa0 deleted the making_it_clear branch December 30, 2023 09:55
kazarmy added a commit to kazarmy/rizin that referenced this pull request Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it clear the "details" of command are available.
4 participants