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

Add test for JSON marshaling for Teams #1941

Merged
merged 2 commits into from
Jul 8, 2021

Conversation

emiscode
Copy link
Contributor

@emiscode emiscode commented Jul 7, 2021

Resource covered: Teams

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 7, 2021
@gmlewis gmlewis changed the title Add test for JSON marshalling for Teams Add test for JSON marshaling for Teams Jul 7, 2021
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @emiscode !
Just a couple minor tweaks, please.

github/teams_test.go Outdated Show resolved Hide resolved
github/teams_test.go Show resolved Hide resolved
@gmlewis gmlewis requested a review from wesleimp July 7, 2021 14:17
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @emiscode !
LGTM.

In future PRs, please do not use force-push, as it makes it more difficult to review changes made since the last code review. We always use squash-and-merge in this repo, so you can push as many commits as necessary, and they will be merged into one final commit.

Awiating second LGTM before merging.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Whups, it looks like the code needs to be run through gofmt first, in order for the tests to pass. Please fix.

- Adjusted commit as per reviewer feedback and run go fmt
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #1941 (8604bc1) into master (05e95d3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1941   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         105      105           
  Lines        6786     6786           
=======================================
  Hits         6627     6627           
  Misses         86       86           
  Partials       73       73           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05e95d3...8604bc1. Read the comment docs.

@emiscode emiscode requested a review from gmlewis July 7, 2021 15:47
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @emiscode !
LGTM.

Awaiting second LGTM before merging.

@emiscode
Copy link
Contributor Author

emiscode commented Jul 7, 2021

Thank you @gmlewis!

I'm glad to be contributing to this project, and I really appreciated your care with details during your review process.

Copy link

@Parker77 Parker77 left a comment

Choose a reason for hiding this comment

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

LGTM.

@gmlewis gmlewis merged commit 55a9b44 into google:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants