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 lab 9 - JSON Puppies #667

Closed
wants to merge 2 commits into from
Closed

Add lab 9 - JSON Puppies #667

wants to merge 2 commits into from

Conversation

alextmz
Copy link
Collaborator

@alextmz alextmz commented Sep 15, 2019

Fixes #666 (nice number)

Review of colleague's PR #489

Changes proposed in this PR:

  • Add Lab 9 - JSON Puppies

@alextmz alextmz changed the title [WIP] Add lab 9 - JSON Puppies Add lab 9 - JSON Puppies Sep 22, 2019
@codecov
Copy link

codecov bot commented Sep 22, 2019

Codecov Report

Merging #667 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #667    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         285    289     +4     
  Lines        5899   6057   +158     
======================================
+ Hits         5899   6057   +158
Impacted Files Coverage Δ
09_json/alextmz/pkg/puppy/errors.go 100% <100%> (ø)
09_json/alextmz/pkg/puppy/store/mapstore.go 100% <100%> (ø)
09_json/alextmz/pkg/puppy/store/syncstore.go 100% <100%> (ø)
09_json/alextmz/cmd/puppy-server/main.go 100% <100%> (ø)

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 b79a437...bf20268. Read the comment docs.

Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

please add colleague's PR review link and pull forward all changes from lab 6/7

09_json/alextmz/cmd/puppy-server/main.go Outdated Show resolved Hide resolved
09_json/alextmz/pkg/puppy/errors.go Outdated Show resolved Hide resolved
09_json/alextmz/pkg/puppy/store/syncstore.go Outdated Show resolved Hide resolved
09_json/alextmz/pkg/puppy/store/syncstore.go Outdated Show resolved Hide resolved
09_json/alextmz/pkg/puppy/types_test.go Outdated Show resolved Hide resolved
}
}

func TestUnmarshall(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

usually we don't test third party / std libraries. json.Unmarshal & json.Marshal work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't get this one - agree that it doesn't make much sense but it's a requirement from Lab 9, unless I interpreted it wrong.

t.Run(name, func(t *testing.T) {
got, err := json.Marshal(test.obj)
assert.NoError(t, err)
assert.JSONEq(t, test.jsn, string(got))
Copy link
Contributor

Choose a reason for hiding this comment

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

usually we don't test third party / std libraries. json.Unmarshal & json.Marshal with the correct json tags in the struct definition are well tested and work. There is no need for you to roll your own JSONstr method IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About the json.Marshall and Unmarshal, see above.
For the JSONstr, I made it a lambda instead - having a custom formatter allows me to keep all the fixtures in one place instead of duplicating them all over. Is this bad?

09_json/alextmz/test/invalid-format.json Outdated Show resolved Hide resolved
09_json/alextmz/test/invalid-ids.json Outdated Show resolved Hide resolved
09_json/alextmz/test/valid-formatted-json.json Outdated Show resolved Hide resolved
Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

sorry haven't looked in detail - can you see if you can make the failing build pass first please?

Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

minor change around testing...

var p Puppy
err := json.Unmarshal([]byte(test.jsn), &p)
assert.NoError(t, err)
assert.JSONEq(t, jsonstr(test.obj), jsonstr(test.obj))
Copy link
Contributor

Choose a reason for hiding this comment

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

wanted and got are the same: jsonstr(test.obj)
To test Unmarshal it should probably be something like:

var p Puppy
err := json.Unmarshal([]byte(test.jsn), &p)
assert.NoError(t, err)
assert.Equal(t, test.obj, p)

And yes, you are right, this is a lab requirements.
What a stupid requirement. Who wrote them? Oh yes that was me.... NVM :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Woops, indeed that won't test much :)

D'oh, I was thinking of assert.Equal as 'taking strings' and not 'taking interfaces'... 🤦‍♂️

Both fixed.


type typetest map[string]struct {
obj Puppy
jsn string
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I should have picked up on this earlier.
I'd probably rename this type as:

type testcases map[string]struct {
	puppy Puppy
	puppyJSON string
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

Nearly there - could you please fix up your commit messages?

Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

almost there...
re commit messages:
https://github.com/anz-bank/go-course/wiki/Lab-1:-Collected-Code-Review-Comments#commit-messages

Commit messages

https://chris.beams.io/posts/git-commit/

Use git rebase -i COMMIT-HASH to rework your commits

  • Separate subject from body with a blank line
  • Limit the subject line to 60 characters
  • Use the imperative mood in the subject line
  • Do not end the subject line with a period
  • Wrap the body at 80 characters
  • Use the body to explain what and why vs. how

E.g. Change

Lab 1 completed.
Built fib code to binary

to

Complete Lab 1
Build fib code to binary

Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

lgtm

@juliaogris
Copy link
Contributor

The go-course is now closed. Thank you very much for participating.

@juliaogris juliaogris closed this Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lab 9 - JSON Puppies
3 participants