Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

MM-47355: Added multierr for postListCmdF (#21222) #564

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

witjem
Copy link
Contributor

@witjem witjem commented Oct 6, 2022

Summary

Adds error handling for postListCmdF. The errors are grouped with multierror.Error package and returned once the loop is done.

Ticket Link

Fixes mattermost/mattermost#21222

@mattermod
Copy link
Contributor

Hello @witjem,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

This looks good, wondering if you can update the tests as well?

@isacikgoz isacikgoz self-requested a review October 18, 2022 11:30
Copy link
Contributor

@noxer noxer left a comment

Choose a reason for hiding this comment

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

Hi @witjem, thank you very much for your contribution. Please update the tests to check for the error. I've also got a small request for a change. Other than that it looks very good.

commands/post.go Outdated Show resolved Hide resolved
@witjem witjem force-pushed the MM-47355-improve-postListCmdF branch from 6749fa5 to f9bfa75 Compare October 26, 2022 20:44
@witjem
Copy link
Contributor Author

witjem commented Oct 26, 2022

@isacikgoz

This looks good, wondering if you can update the tests as well?

I tried write test for it, but for this need mock websocket client. I can do it, but I think for this PR will be a lot changes for this issue, and maybe better create new issue for it?

@isacikgoz
Copy link
Member

I tried write test for it, but for this need mock websocket client. I can do it, but I think for this PR will be a lot changes for this issue, and maybe better create new issue for it?

You don't need to write tests from scratch, just need to reflect the changes you made in the test file so that tests get pass. Because the function now returns an error.

Copy link
Contributor

@noxer noxer left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. Updated tests would still be nice.

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@witjem
Copy link
Contributor Author

witjem commented Nov 7, 2022

@noxer Which tests I should updates? All tests is pass)

Copy link
Contributor

@noxer noxer left a comment

Choose a reason for hiding this comment

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

I meant adding tests for the new errors. I agree that in this case it might not be that easy.
LGTM

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

LGTM, agreed testing can be a bit complicated. Thanks @witjem 🎉

@isacikgoz isacikgoz added the 4: Reviews Complete All reviewers have approved the pull request label Nov 8, 2022
@isacikgoz isacikgoz merged commit d9fdbff into mattermost:master Nov 8, 2022
@witjem witjem deleted the MM-47355-improve-postListCmdF branch November 8, 2022 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mmctl: Review postListCmdF function to return an error in case of a failure
4 participants