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

Fix print_usage and man page mistakes #103

Merged
merged 25 commits into from Nov 23, 2023
Merged

Fix print_usage and man page mistakes #103

merged 25 commits into from Nov 23, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 26, 2023

Usage string is off and this PR fixes it.

4G3NT added 2 commits June 26, 2023 10:29
Seems like someone confused tldr-c-client with another client because -v prints out version and you can't even search for a string instead you have to provide a [PAGE]
@SethFalco
Copy link
Member

Just for context, I checked what the client specifications had on this just to make sure we're changing the correct thing.

Indeed, -v should be for --version.

@ghost ghost changed the title Fix print_usage mistakes Fix print_usage mistakes and man page mistakes Jun 26, 2023
Copy link
Member

@blueskyson blueskyson left a comment

Choose a reason for hiding this comment

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

Look good to me. Maybe @MasterOdin or @Leandros can take a look.

@ghost
Copy link
Author

ghost commented Jul 23, 2023

Turns out that other options, such as --verbose and --version, do exist, on top of -l being non-existent.

img

It appears that using %-30s for padding is unnecessary in this context, as %s alone seems to work perfectly without any issues.
@ghost
Copy link
Author

ghost commented Jul 24, 2023

Hopefully this will be the last commit. 🙂

@ghost
Copy link
Author

ghost commented Jul 24, 2023

If I have time, I can rewrite the arguments so that --version is the only flag, and -v, --verbose is used for verbose.

@ghost
Copy link
Author

ghost commented Jul 24, 2023

Not sure what to do with the current usage string. Maybe it should be:

tldr [OPTION] PAGE

or tldr [ --verbose] [-c|-u] [OPTION]... PAGE

But the problem is it doesn't cover options like --render, which takes a file path, for example.

@ghost
Copy link
Author

ghost commented Jul 27, 2023

Reverting to the old usage but using tldr [--verbose] [OPTION]... PAGE is much better IMO.

@kbdharun kbdharun requested a review from MasterOdin August 20, 2023 04:41
@kbdharun kbdharun changed the title Fix print_usage mistakes and man page mistakes Fix print_usage and man page mistakes Oct 31, 2023
@kbdharun kbdharun marked this pull request as draft October 31, 2023 03:37
@kbdharun kbdharun marked this pull request as ready for review October 31, 2023 03:53
@kbdharun kbdharun requested a review from SethFalco October 31, 2023 04:18
@kbdharun kbdharun marked this pull request as draft October 31, 2023 04:28
@kbdharun kbdharun marked this pull request as ready for review October 31, 2023 04:35
@kbdharun kbdharun requested review from sbrl and removed request for acuteenvy November 12, 2023 02:56
Copy link
Member

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

Just a quick comment or two about behaviour when writing the help. If you do e.g. ls --help, it exits with code 0. This seems to be a well established pattern, so I'd recommend we do the same here in the Node client.

print_version(argv[0]);
return EXIT_SUCCESS;
print_usage(argv[0]);
return EXIT_FAILURE;
Copy link
Member

Choose a reason for hiding this comment

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

I assume this exits with code e.g. 1 instead of 0 when printing help? Other commands e.g. ls, tar, etc exit with code 0 when printing help, so we should do the same.

Unless I misunderstood this?

Copy link
Member

@kbdharun kbdharun Nov 12, 2023

Choose a reason for hiding this comment

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

Our client specification v1.4 suggests to use a non-zero error code for finding pages but doesn't cover the usage part. (tldr-pages/tldr#4246)

Feel free to update the exit code in this branch.

Copy link
Member

Choose a reason for hiding this comment

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

* Specifically, a client MUST exit with a non-zero exit code if the page could not be found anywhere.

Copy link
Author

Choose a reason for hiding this comment

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

I assume this exits with code e.g. 1 instead of 0 when printing help? Other commands e.g. ls, tar, etc exit with code 0 when printing help, so we should do the same.

Unless I misunderstood this?

This exits with 1 on a getopt error.

For example:

  • tldr -a, which is an unknown argument, exits with 1 and displays a getopt error.
  • tldr -v exits with 0 and displays the version.

Copy link
Author

Choose a reason for hiding this comment

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

You can even test this by running these commands:

$ tldr -a 2> /dev/null; echo $?
1
$ tldr -v > /dev/null; echo $?
0

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay. So can you confirm that when you call e.g. tldr --help it exits with code 0, @4G3NT?

Copy link
Author

Choose a reason for hiding this comment

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

Ah okay. So can you confirm that when you call e.g. tldr --help it exits with code 0, @4G3NT?

Yes, it exits with code 0 because of this code:

if (help_flag) {
    print_usage(argv[0]);
    return EXIT_SUCCESS;
}

Copy link
Member

Choose a reason for hiding this comment

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

Very cool, many thanks!

break;

case '?':
/* do not set the help flag, only show getopt error */
/* help_flag = 1; */
return EXIT_FAILURE;
Copy link
Member

Choose a reason for hiding this comment

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

See below comment about exiting with code 0 instead of 1 being preferable when printing help.

@ghost
Copy link
Author

ghost commented Nov 20, 2023

And now when you invoke tldr with -r it will output:

$ tldr -r
./tldr: option requires an argument -- 'r'

instead of the last code:

$ tldr -r
tldr: option '-render' requires an argument

@sbrl
Copy link
Member

sbrl commented Nov 22, 2023

Not sure who is generally responsible for the c client but this seems ready to merge for me.

@kbdharun kbdharun merged commit d0c473f into tldr-pages:main Nov 23, 2023
4 checks passed
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.

-l option for tldr command is ambiguous, inconsistent with help message
4 participants