-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 cases for JSON resource marshaling #1916
Conversation
The types of the fields of the EditChange struct were defined inline as anonymous structs. This made it tedious to use the EditChange struct in testcases, because the anonymous type needed to be redefined (incl the exact same JSON tags) whenever it was assigned. The TestEdit* testcases are good example for it. Using anonymous struct also does not scale well when the struct changes. For example when a field is added to an anonymous struct, every place where the anonymous struct is assigned must also be changed. Another advantage of using regular types is that accessors will be generated automatically and can be used instead of having to manually check for nil values when accessing the struct fields. Define the types as regular structs in event_types.go instead.
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #1916 +/- ##
==========================================
+ Coverage 97.65% 97.84% +0.18%
==========================================
Files 105 105
Lines 6786 6809 +23
==========================================
+ Hits 6627 6662 +35
+ Misses 86 80 -6
+ Partials 73 67 -6
Continue to review full report at Codecov.
|
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.
Thank you, @sagar23sj !
LGTM.
Awaiting second LGTM before merging.
…ucts Fix wrong struct name in godoc comment
This fixes the following golint warning: update-urls/main.go:658:1: receiver name ep should be consistent with previous receiver name e for Endpoint
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.
…into test-cases/activity
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Hi @gmlewis, somehow i managed to ruin the PR while trying to rebase current branch with the latest merges on master. Steps i performed :
|
Hmmm... this is now pretty challenging to fix and not worth the effort. How about closing this PR and creating a new one with the two test additions for:
? |
I can do that @gmlewis . Earlier I was thinking of force push the changes, but I read your comments on other PRs where you mentioned that it doesn't allow you to see the changes, so I didn't force push the changes. I will close this PR then and raise a new one, but can you tell me how do i resolve merge conflicts for other PRs?? |
I'm away from my computer this weekend, but you can try a force push if you wish, or close and create new PRs if you wish... whichever is easier. As for the other ones, creating new PRs might be just as easy as trying to merge. Your call. |
Ok @gmlewis . For the ruined PRs i will raise new ones and for other PRs I'll see what I can do. Thanks and have a great weekend |
Resources Covered :
Helps with issue: #55