-
Notifications
You must be signed in to change notification settings - Fork 25
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 findmeetings typo #38
Fix findmeetings typo #38
Conversation
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.
Good catch 👍
@mickmister I noticed that some of the commands found in /~https://github.com/mattermost/mattermost-plugin-mscalendar/blob/master/server/command/command.go#L61-L88 are not listed in the hint. Should they be added? |
@hanzei Yes, it seems @MatthewDorner Do you mind including these in your changes here? |
There is no help command either, or descriptions of the parameters the subcommands accept such as |
@MatthewDorner How about something like this:
These would also need to show up in the output of We can break this out into a separate ticket since it is growing in scope. What do you think @MatthewDorner? |
We can make it a separate issue. I will just add the missing commands for this PR. |
f88d416
There is a
This led me to think there was no help text, but actually it's just implemented elsewhere in command/help.go (even though it needs to be updated as noted in this thread).
Anyway if |
@MatthewDorner I think it makes sense to have |
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.
LGTM
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.
Sounds nice. Nevertheless, (and probably out of the scope of this PR) what about making the commands case-insensitive? It would mean just a strings.ToLower before the switch.
Tested, this works |
Summary
There is a typo in the autocomplete description for the
/mscalendar
command, and since the command is case-sensitive it will not work if typed as shown. It should befindmeetings
. I didn't find any other instances of this typo in the repository.