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

Bots #25

Merged
merged 9 commits into from
Jun 6, 2019
Merged

Bots #25

merged 9 commits into from
Jun 6, 2019

Conversation

hanzei
Copy link
Collaborator

@hanzei hanzei commented Jun 3, 2019

This PR:

  • Fixes the modules name
  • Migrates to bot accounts
  • Refactors some code in server
  • Adds additional error handling

The best way to review the code is my reviewing the individual commits.

If the refactoring changes are appreciated, I can also apply them to server/zoom.

Fixes #21

@hanzei hanzei requested review from crspeller and levb June 3, 2019 22:02
@hanzei hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Jun 3, 2019
@hanzei hanzei requested a review from jasonblais June 3, 2019 22:02
@hanzei
Copy link
Collaborator Author

hanzei commented Jun 3, 2019

The quality of assets/profile.png is pretty bad. Is there a different file I can use?

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

The changes outlined in the PR description look good. Thanks @hanzei!

@jasonblais
Copy link
Contributor

The quality of assets/profile.png is pretty bad. Is there a different file I can use?

Tagging @asaadmahmood for this question

@hanzei hanzei removed the 1: PM Review Requires review by a product manager label Jun 3, 2019
@asaadmahmood
Copy link
Contributor

@hanzei Here's one with a larger resolution.
Zoom

Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

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

Looks great! And yes the refactoring is much appreciated.

Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

with 1 nit

@@ -31,15 +32,20 @@ func TestPlugin(t *testing.T) {

str, _ := json.Marshal(user)

w.Write([]byte(str))
if _, err := w.Write(str); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: save the if, require will panic if err anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you know this?

@levb levb merged commit 2366a89 into master Jun 6, 2019
@levb levb deleted the bots branch June 6, 2019 15:57
@levb levb added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 6, 2019
@hanzei hanzei mentioned this pull request Jun 13, 2019
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.

Migrate Zoom plugin to use bot accounts
5 participants