Merge lp:~chipaca/ubuntu-push/whoopsie-identifier into lp:ubuntu-push
Proposed by
John Lenton
Status: | Merged |
---|---|
Approved by: | John Lenton |
Approved revision: | 47 |
Merged at revision: | 46 |
Proposed branch: | lp:~chipaca/ubuntu-push/whoopsie-identifier |
Merge into: | lp:ubuntu-push |
Diff against target: |
271 lines (+249/-0) 4 files modified
whoopsie/identifier/identifier.go (+66/-0) whoopsie/identifier/identifier_test.go (+43/-0) whoopsie/identifier/testing/testing.go (+70/-0) whoopsie/identifier/testing/testing_test.go (+70/-0) |
To merge this branch: | bzr merge lp:~chipaca/ubuntu-push/whoopsie-identifier |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Samuele Pedroni | Approve | ||
Review via email: mp+201689@code.launchpad.net |
Commit message
A thin wrapper around libwhoopsie/
Description of the change
A thin wrapper around libwhoopsie/
To post a comment you must log in.
* two levels of dirs/packages in case this gets extracted into a real whoopsie package?
* I would try to standardize on using gocheck early
* at least identifier.go misses the adjusted LICENSE header,
* given that the code introduces an interface Id, is not completely clear to me why conditional compilation
is needed, we could have a SettableIdentifier?
* the code isn't checking that the types are respecting the interface, they aren't because I think it's mixing defining methods on * and non-pointer.
one can do that verifying by either using Id as the return type of New and Failing, or by having things like this in the test or code
var _ Id = Identifier{}
etc
* package, main types are missing docs
* 50 + return Identifier{""}
"" is the zero-value of strings so is optional there
* if we go the conditional compilation route coverage-html needs changes too
* as far as I understand is not the style to call receivers self (fwiw it seems it was at some point), "id", "i" would be more the style