-
Notifications
You must be signed in to change notification settings - Fork 83
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
simple recovery function added #388
simple recovery function added #388
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.
Thanks so much for this contribution @tschuyebuhl 🎉
Functionally LGTM. Just one non-blocking suggestion for an improvement
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.
Nice work, one suggestion below.
Thanks for the comments. I really appreciate it. Addressed both of them, hope that this is a step in the right direction and that the type assertion is not a problem. |
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.
Awesome 🎉
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
- Coverage 33.03% 32.93% -0.10%
==========================================
Files 22 22
Lines 4002 4014 +12
==========================================
Hits 1322 1322
- Misses 2549 2561 +12
Partials 131 131
☔ View full report in Codecov by Sentry. |
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, just one remaining suggestion to explicitly name the *model.AppError
as appErr
instead of err
Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>
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.
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Thanks you @tschuyebuhl 🎉 |
Thank you guys!!! it's been an honor!! |
Summary
I added a simple recovery function and named return parameters to the ExecuteCommand function. Please let me know if this is correct. Would be grateful for any feedback. I'm new to the Mattermost ecosystem, we just recently joined the train in my org.
Ticket Link
This fixes #373.