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 findmeetings typo #38

Merged
merged 3 commits into from
Mar 16, 2020

Conversation

MatthewDorner
Copy link
Collaborator

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 be findmeetings. I didn't find any other instances of this typo in the repository.

hanzei
hanzei previously approved these changes Feb 26, 2020
Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Good catch 👍

@hanzei
Copy link
Collaborator

hanzei commented Feb 26, 2020

@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 hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 26, 2020
mickmister
mickmister previously approved these changes Feb 27, 2020
@mickmister
Copy link
Member

@hanzei Yes, it seems disconnect, connect_bot, and disconnect_bot are missing. testcreateevent should be listed as createevent, as well.

@MatthewDorner Do you mind including these in your changes here?

@mickmister mickmister self-requested a review February 27, 2020 18:36
@MatthewDorner
Copy link
Collaborator Author

MatthewDorner commented Feb 27, 2020

There is no help command either, or descriptions of the parameters the subcommands accept such as subscribe list | show | renew | delete .

@mickmister
Copy link
Member

@MatthewDorner How about something like this:

subscribe (list | show | renew | delete), ...

These would also need to show up in the output of /mscalendar help.

We can break this out into a separate ticket since it is growing in scope. What do you think @MatthewDorner?

@MatthewDorner
Copy link
Collaborator Author

We can make it a separate issue. I will just add the missing commands for this PR.

@MatthewDorner MatthewDorner dismissed stale reviews from mickmister and hanzei via f88d416 February 28, 2020 02:30
@MatthewDorner
Copy link
Collaborator Author

There is a getHelp() placeholder function in command.go:


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 getHelp() won't be used, maybe it should be removed from the code.

@mickmister
Copy link
Member

mickmister commented Feb 28, 2020

@MatthewDorner I think it makes sense to have getHelp() implement the help text, and have c.help call getHelp() in order to use the help text. I'll use this info in the followup ticket.

Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM

@mickmister mickmister requested a review from larkox March 4, 2020 07:00
Copy link
Contributor

@larkox larkox left a 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.

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Mar 4, 2020
@hanzei hanzei requested review from aaronrothschild and DHaussermann and removed request for aaronrothschild March 4, 2020 17:22
@hanzei hanzei added this to the 0.1.0 milestone Mar 7, 2020
@mickmister
Copy link
Member

Tested, this works

@mickmister mickmister merged commit be9a3a8 into mattermost:master Mar 16, 2020
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants