-
Notifications
You must be signed in to change notification settings - Fork 94
make matching case-insensitive on zulip #2115
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
Conversation
let me know if this makes sense, I thought I wouldn't open an issue since it's such a small change, just making the changes and discussing it here made more sense to me :) |
I don't think we currently have any zulip commands that cares about it's argument capitalization, so this would be technically fine. I still wonder if it wouldn't be better to only lowercase the very first letter of the command. cc @Kobzol |
We currently do not, though I just realized that commands that take parameters probably shouldn't get their parameter lowercased. So maybe we should also only do the first word in the arguments, or only do so after the unchanged version failed to parse |
Both make sense to me. The first one would be simpler to implement. |
I think that most of our parameters should already be case resistant. But wouldn't using this: https://docs.rs/clap/latest/clap/struct.Arg.html#method.ignore_case be less of a hack? |
I didn't knew about that setting, it indeed seems much better. |
Sadly AFAIK it cannot be currently applied to the whole command, so we need to add it to each argument separately... |
I looked for that but didn't find it earlier. Would be nice to apply globally ye... |
@jdonszelmann still interested in this change? if not I can do the change with clap ;) |
Am interested, just didn't have time. Go right ahead and feel free to r me for review? |
Hum, Looking around at other command line parser I haven't found any of them who support case insensible commands. Least bad way might be to just eat it and use |
That's what I meant (us ignoring the casing).
That could do it as well, but it's more involved (if even accepted by clap maintainers). |
After some back and forth with the clap maintainer in clap-rs/clap#6097 about adding an ignore case for commands, the clap maintainer is proposing a workaround where we add aliases for each commands with the first letter upper-cased. I think only having the first letter upper-cased would still work for us given how mobile phone keyboards work. That means both We would use the newly added What do you think @jdonszelmann? |
Seems reasonable to me? Neat, thanks for looking into this :3 |
Co-authored-by: Urgau <[email protected]>
a735df1
to
20f335f
Compare
I have updated your PR to make use of the proposed workaround, which work fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty cool solution :)
I often use
work show
from my phone, which automatically capitalizes the first letter in my sentence. I see no reason why we would care about case in these commands, so just converting everything to lowercase shouldn't make a difference to most people while it makes the bot do the right thing more often.