Thanks for your comments. Sorry it's taken so long for me to get back to you. Responses inline:

i think this approach of first tagging the arguments that are matched and then generating the hint for the reminder of the line based on these booleans is too limited.

Regarding the general approach, I'm open to suggestions. I haven't been able to come up with anything dramatically better. We have to match up arguments with input words in order to know which words and which arguments have been consumed and shouldn't be hinted. And we have to generate a hint describing any arguments which might still be matched. Some of the edge cases are tricky, though, and you've found a few of them.

(Nitpick: The matching isn't boolean, it's numeric, since one argument entry can match more than one input word, and an argument entry can be partially matched, such as a token without its parameter value.)

for instance, if the last match is a token which should be followed by a mandatory argument, we're unable (and won't be able) to handle it properly. for instance typing SET key value EX should have hinted either only <seconds>. or adding [NX | XX] [GET], but currently it hints the seconds last and suggests you add NX after EX. i'd argue that this is worse than the previous one which was easy to see that got broken and stopped responding)

Actually, I thought I was handling this scenario correctly. You seem to have found a bug. I'll investigate.

Your first and third examples are cases I believe I can handle. I'll try to fix them.

The second example is trickier, but it might also be doable. It's a bit problematic, though.

Consider the following partial input:

ZADD k IN

This is presumably the start of the token INCR, but it hasn't been matched yet. Do we want to guess this and suggest the completion of the word INCR? We currently only match whole words, not prefixes.

But ZADD k 3 is presumably the start of a score, which we could know only by trying to validate the numeric data type - so far the code doesn't do any type checking.

And, of course, if the next mandatory argument was string-valued, we'd have no way to know whether IN was the start of INCR or the next mandatory string argument.

• what exactly do we do now for older servers (i didn't bother to check yet).

The code currently falls back to the previous hinting algorithm if no arg specs are available.

• do we wanna re-generate help.h so that we can do fancy things even with older servers?

Interesting. Is it that common that an upgraded CLI is run with an old server? If it's worth the effort, that can be left for a future task.

regarding the mandatory arguments for tokens, i don't see how you could handle these, but please try to fix the bug and i'll review.
maybe that would be acceptable.

regarding optional arguments coming before mandatory ones, maybe we could have some code to realize we've hit a mandatory argument, and go mark all the optional ones before it as irrelevant (or keep a pointer to the first argument that's relevant going forward).

Okay, I think I've fixed the cases we discussed earlier. Let me know what you think of them, and the behavior in other situations.

I've also added a test suite for the hint mechanism.

(I also seem to have messed up my branch of the repo, but I'll fix that later... sorry...)

haven't reviewed the code yet, but i did give it a quick test.
so first off, it does work a lot better! thank you!

here are a few places were i see room for improvement:

• 
  this works good (typing p doesn't yet do anything)

• 
  but then, i add x and it hides everything (since it's matching px, but it could also be pxat, so i'd rather do the matching only after hitting space (i.e. exclude the last non-terminated word from the matching and add it only when the user presses space).

• 
  for some reason, when i type an unrecognized keyword, it messes up the rest of the matching, maybe that's avoidable.

• 
  if i don't "consume" the ANY arg, it keeps hunting me like a ghost, even if i already moved on and it's no longer valid.

• 
  if i type an incomplete keyword here (i.e. the n), it assume it's a score and hides all the optional args.

• 
  and then when i add the x of nx it recognizes it's not a score, and re-adds all the optional args.

• 
  when i type the initial score, it suggests member next, but when when i type another score, it still suggests [score member]
  ideally, it would suggest member [score member]. but that's petty, so just a small room for improvement, certainly not a must.

Thanks - those are some great examples. I'll go through them and see what can be improved. Will update you when there's news.

i'd rather do the matching only after hitting space

Do you mean in general? Only match the last word after the user hits space? That's not the behavior of the old hints. They appear as soon as the command name is complete, and then they match each word as soon as its first character is typed.

We can change that behavior in general, and maybe we should. But it would be a change from the current behavior.

for positional arguments, maybe we can mark them as matched as soon as one character is typed.
but for optional args (specifically when there's more than one option), it could be a problem.
what bothered me specifically was when you type PX, it immediately hides all the options and shows "milliseconds", but it could still be PXAT.

i'm not certain, but i feel it might be better.

• 
  but then, i add x and it hides everything (since it's matching px, but it could also be pxat, so i'd rather do the matching only after hitting space (i.e. exclude the last non-terminated word from the matching and add it only when the user presses space).

Most of the glitches you point out in this comment can be solved by deciding that we only match words after the space is typed. I've made that change and I prefer the behavior that way. I'm inclined to adopt that rule in general.

• 
  for some reason, when i type an unrecognized keyword, it messes up the rest of the matching, maybe that's avoidable.

I'm not sure that's avoidable. The unrecognized keyword is not valid. I think the only solution would be to stop hinting if an unmatchable word is entered.

• 
  if i don't "consume" the ANY arg, it keeps hunting me like a ghost, even if i already moved on and it's no longer valid.

This one is tricky. I need to think a bit more about how to fix it.

when i type the initial score, it suggests member next, but when when i type another score, it still suggests [score member]
ideally, it would suggest member [score member]. but that's petty, so just a small room for improvement, certainly not a must.

I agree that it's not ideal, but I don't think it's easy to solve and I'm not inclined to spend time on it for now.

@jhelbaum i took anther quick look (mostly at the tests, not the code).
AFAICT the main remaining issue that's hard to overlook is the problem with the ANY after COUNT in GEORADIUS.

regarding the other issues.

• i think that after an unrecognized input, we can simply stop hinting (might be better than showing irrelevant options).
• the problem i mentioned about ZADD is arguably even more annoying in BITFIELD after repeating the first action

I agree the risk for issues is small but still don't like to merge a mass of unreviewed code.
Maybe the level of review you did is enough?
I suppose there's no need to explicitly look for bugs, but I think we do need to make sure it is constructed in a readable manner and includes enough comments to be maintainable.

Yeah, I didn't execute the code in my head, but the code looks nice, seems to have enough comments, fairly short functions, comprehensive variable names, etc. I would say it's probably maintainable.

Yes, thanks for taking a look at this. I'm unlikely to have much time to devote to this at the moment, however.

Regarding maintenance, I would just note that the test suite has a minor maintenance burden in that it is dependent on the current command specs. If some of the command argument specs change, the test cases for those commands will have to be updated accordingly (since the hint strings will change).

It might be "better" in some ideal world for the tests to be based on some static set of command specs, but that's not realistic to implement IMHO.

In any case, it would be nice to see this code merged. I think it's a handy feature.

@zuiderkwast @jhelbaum i gave it some thought and also consulted Yossi and Itamar, i decided to take Viktor's advise and merge it without another reviewer, considering this is an isolated area, and we can fix things or improve readability later.
So now it'll just take someone to defrost this PR and bring it back to mergable state.
@zuiderkwast maybe you wanna have a go at that? (since Jason said he doesn't currently have the time)

Defrost? 😁 Only fix merge conflicts and possibly address my own review comments, right?

yes, plus any additional cleanup you can do which results of a another pair of eyes reading this (like clarifying come comments)

OK, I'll do it. 👍

btw, for the technical part, maybe @jhelbaum can grant you access to his branch, or you can make PRs to it.

🎉 🎉 🎉 @jhelbaum merged thank you