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

--help me if you can #5680

Closed
wants to merge 5 commits into from
Closed

Conversation

@emilengler
Copy link
Contributor

emilengler commented Jul 14, 2020

Introduction

This implements the --help me if you can by Daniel Fandrich presented at curl-up 2020. See here.
I know that the diff looks big (242 files changed). However, ~99% of these just set the category for each item.
I suggest you to have a look at this YouTube Video where I explain how this works in detail.
I hope this can get into the next release. Maybe set this high priority?

I split up curl into 22 categories:

 auth                Options related to authentication of any kind
 connection          Options related to low level network operations
 curl                Options related to the behaviour of curl itself
 dns                 Options related to DNS
 file                Options related to the FILE protocol
 ftp                 Options related to the FTP protocol
 http                Options related to the HTTP and HTTPS protocols
 imap                Options related to the IMAP protocol
 misc                Options that don't fit in any other category
 output              Options that handle the output of the downloaded content
 pop3                Options related to the POP3 protocol
 post                Optons related to HTTP Post
 proxy               Options related to proxy settings of any kind
 scp                 Options related to the SCP protocol
 sftp                Options related to the SFTP protocol
 smtp                Options related to the SMTP protocol
 ssh                 Options related to the SSH protocol
 telnet              Options related to the Telnet protocol
 tftp                Options related to the TFTP protocol
 tls                 Options related to any kind of TLS/SSL operation
 upload              Options related to uploads of any kind
 verbose             Options related to any kind of command line output of curl

It works through a bitmask. Because of this every item can contain multiple categories.
If the user does not request any special category, a list of the most important curl commands is being shown. I know this topic is highly controversial and I am open to any suggestions (maybe also start a user survey?).

Reviewing

Please see this PR as two parts. One documentation part and one technical part.
The documentation part is just the first commit (d76538c).
The technical part are all commits after this one.
The technical part is already ready to merge in my opinion. However the documentation part still needs to be reviewed a lot. I put most options into the categories on guessing.

Requested Reviewers

Of course everyone is most welcome to do his review, I would still like a review from @bagder on the selected categories (sorry for 200+ files :P) as he probably knows them the best and a review from @dfandrich as he is the inventor of this idea.

TODO

  • The advanced help menu logic
  • Tests
  • Category review
@gvanem
Copy link
Contributor

gvanem commented Jul 14, 2020

Handling the categories are like traversing a directory. So it would be handy to have an interactive completion mode in
curl --help (or whatever) to navigate through all the options (since it's hard to know what to look for).

With the Python prompt_toolkit it's very easy to write fancy prompts like in this little demo:

#!/usr/bin/env python

from prompt_toolkit.shortcuts  import CompleteStyle, prompt
from prompt_toolkit.completion import WordCompleter

curl_completer = WordCompleter ([ "auth", "connection", "curl", "dns", "file", "ftp", "http", "imap",
                                  "misc", "output", "pop3", "post", "proxy", "scp", "sftp", "smtp",
                                  "ssh", "telnet", "tftp", "tls", "upload", "verbose" ],
                                ignore_case = True)

category = prompt ("Give me a curl category. Press <TAB> for all: ",
                   completer = curl_completer, complete_while_typing = True,
                   complete_style = CompleteStyle.MULTI_COLUMN)

print ("Got category: %s" % category)

It looks like this when I press the <TAB>:
curl-cat

The idea was then to give another prompt based on a match to those added Category: lines in cmdline-opts/*.d.
And most users have Python installed nowadays, no?

@emilengler
Copy link
Contributor Author

emilengler commented Jul 14, 2020

Handling the categories are like traversing a directory. So it would be handy to have an interactive completion mode in

curl --help (or whatever) to navigate through all the options (since it's hard to know what to look for).

With the Python prompt_toolkit it's very easy to write fancy prompts like in this little demo:

#!/usr/bin/env python



from prompt_toolkit.shortcuts  import CompleteStyle, prompt

from prompt_toolkit.completion import WordCompleter



curl_completer = WordCompleter ([ "auth", "connection", "curl", "dns", "file", "ftp", "http", "imap",

                                  "misc", "output", "pop3", "post", "proxy", "scp", "sftp", "smtp",

                                  "ssh", "telnet", "tftp", "tls", "upload", "verbose" ],

                                ignore_case = True)



category = prompt ("Give me a curl category. Press <TAB> for all: ",

                   completer = curl_completer, complete_while_typing = True,

                   complete_style = CompleteStyle.MULTI_COLUMN)



print ("Got category: %s" % category)

It looks like this when I press the <TAB>:

curl-cat

The idea was then to give another prompt based on a match to those added Category: lines in cmdline-opts/*.d.

And most users have Python installed nowadays, no?

I like your idea, however making it interactive is the task of the shell, not ours. However maybe we should provide autocompletion scripts. It wouldn't be a problem to add this functionality to the gen.pl script but this is probably outside of the scope of this PR.

@bagder
Copy link
Member

bagder commented Jul 14, 2020

Test 1139 seems to fail on some CI builds (example):

 --hostpubmd5 is not in curl.1 (but in tool_getparam.c tool_help.c)
@bagder
Copy link
Member

bagder commented Jul 14, 2020

Two questions (that I didn't find answered yet, most probably because I didn't look very closely yet):

  1. Will it cause a build error if I leave out a category for a cmdline option? I think it should.
  2. Will it cause a build error if I misspell a category in the cmdline docs file? I think it should.
@emilengler
Copy link
Contributor Author

emilengler commented Jul 14, 2020

Will it cause a build error if I leave out a category for a cmdline option? I think it should.

Not yet but I think it throws a warning that it misses a category line. Same with other required options

Will it cause a build error if I misspell a category in the cmdline docs file? I think it should.

Yes, because there won't be a bitmask #DEFINE. However if you're generate the bitmask definitions with ./gen.pl listcats then their won't

@emilengler
Copy link
Contributor Author

emilengler commented Jul 14, 2020

Test 1139 seems to fail on some CI builds (example):

 --hostpubmd5 is not in curl.1 (but in tool_getparam.c tool_help.c)

Fixed in dd873f6

@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from dd873f6 to af4dd10 Jul 14, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Jul 14, 2020

I re-ordered the commits into a more logical order

@@ -2,6 +2,7 @@ Short: a
Long: append
Help: Append to target file when uploading
Protocols: FTP SFTP
Category: ftp sftp

This comment has been minimized.

@sterchelen

sterchelen Jul 17, 2020 Contributor

I would say categories

Suggested change
Category: ftp sftp
Categories: ftp sftp

I see that in most cases, categories is equal to protocols. Does it make sense to use by default the protocols field as the categories and in specific cases use the categories as an override field ?

This comment has been minimized.

@emilengler

emilengler Jul 18, 2020 Author Contributor

Thanks for your review!
Most options only belong to one category. The plural would sound weird there in my opinion.

This comment has been minimized.

@bagder

bagder Jul 18, 2020 Member

Does it make sense to use by default the protocols field as the categories

That sounds like a lever idea that will reduce a lot of duplication!

This comment has been minimized.

@emilengler

emilengler Jul 18, 2020 Author Contributor

That sounds like a lever idea that will reduce a lot of duplication!

But what is with options like --user or --output which don't belong to any (specific) protocol

This comment has been minimized.

@sterchelen

sterchelen Jul 18, 2020 Contributor

Most options only belong to one category. The plural would sound weird there in my opinion.

In your code you are already referencing categories:

curl/src/tool_help.c

Lines 119 to 123 in af4dd10

struct helptxt {
const char *opt;
const char *desc;
curlhelp_t categories;
};

Protocols is in plural form and b/c category(ies) is relatively equal to protocols I think we need to stay consistent.

But what is with options like --user or --output which don't belong to any (specific) protocol

We could you use this logic to define the category(ies):

my_category(ies): category(ies) | default(protocols) | default("curl")

What do you think?

This comment has been minimized.

@sterchelen

sterchelen Jul 27, 2020 Contributor

@bagder I'm not sure to understand you point of view 🤔 ; tell me if I am wrong:

  1. each file has a category: field
  2. and over all just make sure they match, so you have to duplicate the protocols in the categories to make sure it is correct?

This comment has been minimized.

@bagder

bagder Jul 27, 2020 Member

Yes I think:

  • each .d file gets a category: field, that can be empty or list additional categories apart from the protocols which are used by default
  • The category curl should rather be renamed tool and should be explicitly mentioned as a category if desired.

This way, the used categories are all visible in each file (just that they're also in the protocols line) and we don't need to repeat the protocols in the category field.

This comment has been minimized.

@sterchelen

sterchelen Jul 28, 2020 Contributor

Sounds good to me, your position @emilengler 😃 ?

This comment has been minimized.

@emilengler

emilengler Jul 28, 2020 Author Contributor

I will have a deeper look on this at Sunday or Monday. I am currently on vacation

This comment has been minimized.

@emilengler

emilengler Aug 2, 2020 Author Contributor

The change from tool to curl makes sense. I will change this.
I also agree on the part that the category field can be empty where the item would fall into the misc category then.
However I still don't think it is a good idea to mix up with protocols. Of course this saves some work but I still think it adds more confusion also AFAIK the protocol field is not omnipresent so far

src/tool_help.c Outdated Show resolved Hide resolved
src/tool_help.c Outdated Show resolved Hide resolved
src/tool_help.c Outdated Show resolved Hide resolved
@emilengler
Copy link
Contributor Author

emilengler commented Aug 13, 2020

I implemented @bagder's improvements. I will now put the commits into a more logical order

@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch 2 times, most recently from 32cd3f4 to 0237466 Aug 13, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Aug 13, 2020

I've also added a new test which tests the case insensitivity.

@sterchelen
Copy link
Contributor

sterchelen commented Aug 21, 2020

@emilengler a lot of tests are failing.

tool_help.c:891:13: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
 static void get_categories()
@emilengler
Copy link
Contributor Author

emilengler commented Aug 21, 2020

@emilengler a lot of tests are failing.


tool_help.c:891:13: error: function declaration isn’t a prototype [-Werror=strict-prototypes]

 static void get_categories()

Thanks for your testing! For me they all worked fine o.O
How you have run them? Just make test?

@sterchelen
Copy link
Contributor

sterchelen commented Aug 21, 2020

@emilengler a lot of tests are failing.


tool_help.c:891:13: error: function declaration isn’t a prototype [-Werror=strict-prototypes]

 static void get_categories()

Thanks for your testing! For me they all worked fine o.O
How you have run them? Just make test?

By looking on the checks of your PR

@emilengler
Copy link
Contributor Author

emilengler commented Aug 21, 2020

@emilengler a lot of tests are failing.


tool_help.c:891:13: error: function declaration isn’t a prototype [-Werror=strict-prototypes]

 static void get_categories()

Thanks for your testing! For me they all worked fine o.O
How you have run them? Just make test?

By looking on the checks of your PR

I will have a deeper a look soon.

@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from 0237466 to bf24e39 Aug 22, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Aug 22, 2020

Should be fixed now

@emilengler
Copy link
Contributor Author

emilengler commented Aug 23, 2020

Someone knows what type I should use? I need at least 32-bits of space for the bit mask. However fixed size data types are a C99 thing

@bagder
Copy link
Member

bagder commented Aug 23, 2020

You mean for an unsigned 32 bit variable? unsigned int will be good enough. curl only builds on >= 32 bit systems and they all have 32 bit ints...

@emilengler
Copy link
Contributor Author

emilengler commented Aug 24, 2020

You mean for an unsigned 32 bit variable? unsigned int will be good enough. curl only builds on >= 32 bit systems and they all have 32 bit ints...

Ok will change that

@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from bf24e39 to b63bc8a Aug 24, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Aug 24, 2020

Updated

@emilengler
Copy link
Contributor Author

emilengler commented Aug 24, 2020

I think the failed test is very likely a false positive

@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from 7a367a4 to e63de0e Sep 2, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Sep 2, 2020

Merge conflicts solved

@bagder
bagder approved these changes Sep 3, 2020
Copy link
Member

bagder left a comment

I think this looks great. This is a big step forward for --help. I think the categories you've picked seem perfect. We can always fix them later if we find mistakes.

I just have three minor things.

src/tool_help.c Outdated Show resolved Hide resolved
src/tool_help.c Outdated Show resolved Hide resolved
src/tool_help.c Show resolved Hide resolved
@SuperSandro2000
Copy link

SuperSandro2000 commented Sep 3, 2020

Could we get a bash completion script for that? I specially asking not for a command line option cause this either makes she'll startup slow or decouples the auto completion version from the fuel version.

@bagder
Copy link
Member

bagder commented Sep 3, 2020

I generally like the bash completion for curl provided by the bash-completion package on Debian. It is maintained outside of the curl project.

Copy link
Contributor

sterchelen left a comment

eager to see this great addition in the next release 💪

emilengler added a commit to emilengler/curl that referenced this pull request Sep 3, 2020
Closes curl#5680
@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from e63de0e to 682e4a8 Sep 3, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Sep 3, 2020

@bagder Changes from you applied. I also rephrased the category descriptions to be more descriptive.

@emilengler
Copy link
Contributor Author

emilengler commented Sep 3, 2020

Note to the maintainer who will merge this:
Would it be possible to merge the 5 commits as they are without squashing them?
I think it's better to have 5 different commits which each do another part. This is simpler to maintain rather than having one big diff.

@bagder
Copy link
Member

bagder commented Sep 3, 2020

Sure, that's possible. But are they really stand-alone enough to warrant being individual commits? They certainly don't have proper individual commit messages right now...

@emilengler
Copy link
Contributor Author

emilengler commented Sep 3, 2020

Sure, that's possible. But are they really stand-alone enough to warrant being individual commits? They certainly don't have proper individual commit messages right now...

Yes I also thought about that while writing the comment. I thought about adding a prefix or add that line to the commit message:

This commit is a part of "--help-me-if-you-can"; #5680
@emilengler
Copy link
Contributor Author

emilengler commented Sep 3, 2020

The failing tests on Windows are a lower level problems. It is because of line endings. Adding the Windows line ending to the test cases will make them to not work on Windows. Fixing this would be outside of the scope of this PR.

@bagder
Copy link
Member

bagder commented Sep 3, 2020

Fixing it is a matter of setting mode=text" in the <stdout> tag. Like: <stdout mode="text">

emilengler added 5 commits Jul 13, 2020
This is the begin of the implementation of "--help me if you can"
by Daniel Fandrich presented at curl up 2020.
See: https://bit.ly/2C92PvR

This commit is a part of "--help me if you can"; #5680
This commit is a part of "--help me if you can"; #5680
This commit is a part of "--help me if you can"; #5680
This commit is a part of "--help me if you can"; #5680
This commit is a part of "--help me if you can"; #5680

Closes #5680
@emilengler emilengler force-pushed the emilengler:2020-07-help-me-if-you-can branch from 682e4a8 to ac93efd Sep 4, 2020
@emilengler
Copy link
Contributor Author

emilengler commented Sep 4, 2020

Tests should be fixed now

@emilengler
Copy link
Contributor Author

emilengler commented Sep 4, 2020

Sure, that's possible. But are they really stand-alone enough to warrant being individual commits? They certainly don't have proper individual commit messages right now...

I made something that helps in that case. See the commit messages

@emilengler
Copy link
Contributor Author

emilengler commented Sep 4, 2020

I think the CI failures are fine. As far as I think they are unrelated to this PR

@bagder
Copy link
Member

bagder commented Sep 4, 2020

I've taken the liberty of squashing them into three logical commits: docs, tool and tests. Merging now.

Many thanks for your hard work on this!

@bagder bagder closed this in 5dddc1d Sep 4, 2020
bagder added a commit that referenced this pull request Sep 4, 2020
This commit is a part of "--help me if you can"

Closes #5680
bagder added a commit that referenced this pull request Sep 4, 2020
This commit is a part of "--help me if you can"

Closes #5680
@emilengler
Copy link
Contributor Author

emilengler commented Sep 4, 2020

I've taken the liberty of squashing them into three logical commits: docs, tool and tests. Merging now.

Many thanks for your hard work on this!

Was a pleasure for me!

@dfandrich
Copy link
Collaborator

dfandrich commented Sep 7, 2020

@gvanem gvanem mentioned this pull request Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.