-
Notifications
You must be signed in to change notification settings - Fork 164
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
Complete lab9 #489
Complete lab9 #489
Conversation
177be32
to
ad22907
Compare
6a5fe7c
to
672b90e
Compare
d8fd441
to
f074643
Compare
Codecov Report
@@ Coverage Diff @@
## master #489 +/- ##
======================================
Coverage 100% 100%
======================================
Files 157 161 +4
Lines 2806 2926 +120
======================================
+ Hits 2806 2926 +120
Continue to review full report at Codecov.
|
a854acf
to
d6976e2
Compare
8a8e963
to
b1fda19
Compare
One more thing. Marcin. Please fix up your commit message. It should be describing what you are committing. I got caught doing the same thing once like you :) Cheers! |
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.
Let's resolve discussions around monkey patching and error passing in lab10 PR first please.
I've also updated this PR description.
4924259
to
7657dd3
Compare
Copy lab08 Apply some new patches from lab07 Add command line support Add example data in json file
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.
I couple more comments on variable naming / function signatures / structure, but nothing too big.
I'm happy for you to merge as is or fix and ping me personally and I'll approve right away. Up to you :)
|
||
var ( | ||
logFatalf = log.Fatal | ||
parser = parseArgs |
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.
i like this!
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.
TIL
return nil | ||
} | ||
|
||
func parseArgs(args []string) (config, error) { |
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.
nice!
func createStorer(c *config) error { | ||
switch c.sType { | ||
case "sync": | ||
c.storer = store.NewSyncStore() |
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.
I think it would be nicer if this function didn't modify it's input but return the Storer:
func createStorer(storeStr string) (puppy.Storer, error) { //...
To me config should as much as possible only hold read-only/immutable config info (which storer is not).
Also, where all relevant information is just one (or two) fields of the config I find it nicer to pass those few fields (keep the interface smaller).
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.
fixed in lab10
var storeType string | ||
var puppyFile *os.File | ||
kingpin.Flag("data", "path to file with puppies data").Short('d').FileVar(&puppyFile) | ||
kingpin.Flag("store", "Store type").Short('s').Default("map").EnumVar(&storeType, "map", "sync") |
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.
cool i didn't know about this... will remember.
Thanks for the review. I'll merge this as is and implement changes in |
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.
Nice code and some good ideas here. Had to really scratch the bottom of the nit-bag to find stuff to write about.
|
||
var ( | ||
logFatalf = log.Fatal | ||
parser = parseArgs |
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.
TIL
|
||
Review coverage with | ||
|
||
go test -coverprofile=coverage.out ./... && go tool cover -html=coverage.out |
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.
nit: should had specified usage and parameters :-)
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.
not sure if I get it. for sure it's refactored in lab11
logFatalf(err) | ||
} | ||
logFatalf(runPuppyServer(&config)) | ||
} |
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.
That's clean!
kingpin.Flag("data", "path to file with puppies data").Short('d').FileVar(&puppyFile) | ||
kingpin.Flag("store", "Store type").Short('s').Default("map").EnumVar(&storeType, "map", "sync") | ||
_, err := kingpin.CommandLine.Parse(args) | ||
return config{puppyFile, storeType, nil}, err |
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.
That's a much better way than I did
} | ||
var puppies []puppy.Puppy | ||
if err = json.Unmarshal(b, &puppies); err != nil { | ||
return nil, errors.New(string(b)) |
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.
Ok, here you return the read file contents as an error, but then you lose the context of whatever is returned by the Unmarshal
error - might make whoever reads the error message confused, getting just a dump without saying what is wrong
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.
good catch. I have no idea why I didn't just return nil, err
// CreatePuppy creates puppy | ||
func (m *MemStore) CreatePuppy(p *puppy.Puppy) (int, error) { | ||
if p.Value < 0 { | ||
return -1, puppy.Errorf(puppy.ErrCodeInvalidInput, "Puppy value have to be positive number") |
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.
"Puppy value has to be equal or bigger than zero", as zero itself is nor positive nor negative (and I'm really running out of nits here)
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.
?? This is returning error if p.Value is lower than 0. 0 < 0
-> False so we're jumping to line 26. If p.Value
is equal or larger than 0 I'm not throwing error? Am I missing sth?
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.
0 is positive as far as i remember.
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.
Just a nit about the message itself - zero is neither negative nor positive, it's zero. So saying that value has to be positive is not strictly true, because the smallest possible value needs to be zero. So, the message must say value has to be bigger or equal than zero
or sth else that includes the possibility of a zero value. Nit, really... :)
} | ||
|
||
// UpdatePuppy updates puppy | ||
func (m *MemStore) UpdatePuppy(id int, p *puppy.Puppy) error { |
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.
I'd skip passing the ID separately, as Puppy already has one. If you're updating, the ID shouldn't change, so the ID in Puppy should be enough.
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.
This is holly war :)
Example PUT in K8S (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#volumemount-v1-core)
PUT /apis/batch/v1beta1/namespaces/{namespace}/cronjobs/{name}
We could extract {name}
from payload, but it's provided explicitly.
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.
:) Indeed can be a matter of taste. I agree with using it explicitly in case of a PUT, but just doesn't feel right in the case of internal language functions. But it's taste anyway :)
} | ||
|
||
// DeletePuppy deletes puppy | ||
func (m *MemStore) DeletePuppy(id int) (bool, error) { |
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.
Feels like that both bool and error returning a bit too much, isn't just error enough?
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.
yes it is. It's fixed in lab10
or lab11
return puppy.Puppy{ | ||
Breed: "Type A", | ||
Colour: "Grey", | ||
Value: rand.Intn(100) + 100, |
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.
good one randomizing the value
// Puppy contains information about single puppy | ||
type Puppy struct { | ||
ID int `json:"id"` | ||
Value int `json:"value"` |
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.
Value could be 1,99... (float) 😆
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.
design assumption :)
Lab 9 - JSON input
Fixes #495
Review of colleague's PR #474
Changes proposed in this PR:
Implement features requested in spec.