Add lab 10 - Puppy REST #669
Conversation
alextmz
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
Codecov Report
@@ Coverage Diff @@
## master #669 +/- ##
======================================
Coverage 100% 100%
======================================
Files 285 290 +5
Lines 5899 6139 +240
======================================
+ Hits 5899 6139 +240
Continue to review full report at Codecov.
|
|
Woops. |
| out io.Writer = os.Stdout | ||
|
|
||
| // shutdownhttp signals main() to... shutdown the http server | ||
| shutdownhttp = make(chan bool) |
There was a problem hiding this comment.
I believe it's overkill, but let's wait for code owners. You are not dealing with graceful shutdown to justify this
There was a problem hiding this comment.
My intent was to do real connection testing on main(), not just handler testing, and this was a (relatively) elegant way I found to http.Shutdown a http.ListenAndServe; but as I later found out, Travis doesn't allow connections to localhost.
So this could had been substituted for a global http.Server variable, and the test could had http.Shutdown the server it before calling main().
Which also doesn't feel a good solution despite the simplicity.
So I left it in as it works, and doesn't use another global.
I hope reviewers like it but I admit I'm not high on hopes 🤞
| } | ||
|
|
||
| puppy, err := ht.Store.ReadPuppy(id) | ||
|
|
There was a problem hiding this comment.
sometimes you've got new lines here, sometimes you don't have them. This is NIT and can be easily ignored
| n++ | ||
| } | ||
|
|
||
| return n, nil |
There was a problem hiding this comment.
return len(puppies)-1, nil should allow getting rid of n from this function
There was a problem hiding this comment.
Yep, actually len(puppies). Changed!
| return | ||
| } | ||
|
|
||
| jsonfile, err := os.Open(*flagfile) |
There was a problem hiding this comment.
There is a kingpimg flag file type that opens the file as part of command line parsing. saves you the error handling here.
| jsonfile, err := os.Open(*flagfile) | ||
| if err != nil { | ||
| fmt.Fprintf(out, "error opening file: %v\n", err) | ||
| return |
There was a problem hiding this comment.
os.Exit(1) or log.Fatal(msg) ?
| // printpuppies print n puppies contained in the store s | ||
| func printpuppies(s puppy.Storer, n int) { | ||
| for i := 1; i <= n; i++ { | ||
| p, err := s.ReadPuppy(i) |
There was a problem hiding this comment.
You are making assumptions on puppy IDs being a sequence, that might be dangerous imo.
I think it's niver if you just impoment the Stringer interface for puppy, and if you truly want to print a slice of puppies, retrieve them as a list and then maybe fmt.Println(strings.Join(puppies, "\n")) or similar. It is a easier to test correct string creation than it is to test correct printing.
| // test if main() is really serving the HTTP we expect | ||
| // | ||
| // unfortunately, looks like travis-ci does not allow | ||
| // connections to 127.0.0.1: we get 'connect: connection refused' |
There was a problem hiding this comment.
hmm this sounds strange.
I'm pretty sure I recall using httptest.Server which essentially starts a webserver on localhost in tests executed on Travis.
Can you use httptest.Server?
|
The go-course is now closed. Thank you very much for participating. |
Fixes #668
Review of colleague's PR #584
Changes proposed in this PR: