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
Reviewer Review Type Date Requested Status
Samuele Pedroni Approve
Review via email: mp+201689@code.launchpad.net

Commit message

A thin wrapper around libwhoopsie/identifier.h.

Description of the change

A thin wrapper around libwhoopsie/identifier.h.

To post a comment you must log in.
Revision history for this message
Samuele Pedroni (pedronis) wrote :

* 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

review: Needs Fixing
Revision history for this message
John Lenton (chipaca) wrote :

Good points, all of them.

> * two levels of dirs/packages in case this gets extracted into a real whoopsie package?

Are you asking whether that is why I made two levels? (yes). Are you suggesting I add a third level?

About checking interfaces, yes I forgot to do that. I thought you checked interfaces doing

    if ok := myObject.(myInterface); !ok {
        panic and dispair
    }

About SettableIdentifier, you think SettableIdentifier and FailingIdentifier should be exposed in production?

44. By John Lenton

merged trunk

45. By John Lenton

clean up and addressed issues raised by Samuele's review

46. By John Lenton

docs

Revision history for this message
Samuele Pedroni (pedronis) wrote :

looks good but needs a make format pass

review: Approve
47. By John Lenton

make format

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'whoopsie'
2=== added directory 'whoopsie/identifier'
3=== added file 'whoopsie/identifier/identifier.go'
4--- whoopsie/identifier/identifier.go 1970-01-01 00:00:00 +0000
5+++ whoopsie/identifier/identifier.go 2014-01-15 13:48:09 +0000
6@@ -0,0 +1,66 @@
7+/*
8+ Copyright 2013-2014 Canonical Ltd.
9+
10+ This program is free software: you can redistribute it and/or modify it
11+ under the terms of the GNU General Public License version 3, as published
12+ by the Free Software Foundation.
13+
14+ This program is distributed in the hope that it will be useful, but
15+ WITHOUT ANY WARRANTY; without even the implied warranties of
16+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
17+ PURPOSE. See the GNU General Public License for more details.
18+
19+ You should have received a copy of the GNU General Public License along
20+ with this program. If not, see <http://www.gnu.org/licenses/>.
21+*/
22+
23+// Package whoopsie/identifier wraps libwhoopsie, and is thus the
24+// source of an anonymous and stable system id used by the Ubuntu
25+// error tracker and the Ubuntu push notifications service.
26+package identifier
27+
28+/*
29+#cgo pkg-config: libwhoopsie
30+#include <glib.h>
31+#include <libwhoopsie/identifier.h>
32+*/
33+import "C"
34+import "unsafe"
35+import "errors"
36+
37+// an Id knows how to generate itself, and how to stringify itself.
38+type Id interface {
39+ Generate() error
40+ String() string
41+}
42+
43+// Identifier is the default Id implementation.
44+type Identifier struct {
45+ value string
46+}
47+
48+// New creates an Identifier, but does not call Generate() on it.
49+func New() *Identifier {
50+ return &Identifier{}
51+}
52+
53+// Generate makes the Identifier create the identifier itself.
54+func (id *Identifier) Generate() error {
55+ var gerr *C.GError
56+ var cs *C.char
57+ defer C.g_free((C.gpointer)(unsafe.Pointer(cs)))
58+ C.whoopsie_identifier_generate(&cs, &gerr)
59+
60+ if gerr != nil {
61+ return errors.New(C.GoString((*C.char)(gerr.message)))
62+ } else {
63+ id.value = C.GoString(cs)
64+ return nil
65+ }
66+
67+}
68+
69+// String returns the system identifier as a string.
70+func (id *Identifier) String() string {
71+ return id.value
72+}
73
74=== added file 'whoopsie/identifier/identifier_test.go'
75--- whoopsie/identifier/identifier_test.go 1970-01-01 00:00:00 +0000
76+++ whoopsie/identifier/identifier_test.go 2014-01-15 13:48:09 +0000
77@@ -0,0 +1,43 @@
78+/*
79+ Copyright 2013-2014 Canonical Ltd.
80+
81+ This program is free software: you can redistribute it and/or modify it
82+ under the terms of the GNU General Public License version 3, as published
83+ by the Free Software Foundation.
84+
85+ This program is distributed in the hope that it will be useful, but
86+ WITHOUT ANY WARRANTY; without even the implied warranties of
87+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
88+ PURPOSE. See the GNU General Public License for more details.
89+
90+ You should have received a copy of the GNU General Public License along
91+ with this program. If not, see <http://www.gnu.org/licenses/>.
92+*/
93+
94+package identifier
95+
96+import (
97+ . "launchpad.net/gocheck"
98+ "testing"
99+)
100+
101+// hook up gocheck
102+func Test(t *testing.T) { TestingT(t) }
103+
104+type IdentifierSuite struct{}
105+
106+var _ = Suite(&IdentifierSuite{})
107+
108+// TestGenerate checks that Generate() does not fail, and returns a
109+// 128-byte string.
110+func (s *IdentifierSuite) TestGenerate(c *C) {
111+ id := New()
112+
113+ c.Check(id.Generate(), Equals, nil)
114+ c.Check(id.String(), HasLen, 128)
115+}
116+
117+// TestIdentifierInterface checks that Identifier implements Id.
118+func (s *IdentifierSuite) TestIdentifierInterface(c *C) {
119+ _ = []Id{New()}
120+}
121
122=== added directory 'whoopsie/identifier/testing'
123=== added file 'whoopsie/identifier/testing/testing.go'
124--- whoopsie/identifier/testing/testing.go 1970-01-01 00:00:00 +0000
125+++ whoopsie/identifier/testing/testing.go 2014-01-15 13:48:09 +0000
126@@ -0,0 +1,70 @@
127+/*
128+ Copyright 2013-2014 Canonical Ltd.
129+
130+ This program is free software: you can redistribute it and/or modify it
131+ under the terms of the GNU General Public License version 3, as published
132+ by the Free Software Foundation.
133+
134+ This program is distributed in the hope that it will be useful, but
135+ WITHOUT ANY WARRANTY; without even the implied warranties of
136+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
137+ PURPOSE. See the GNU General Public License for more details.
138+
139+ You should have received a copy of the GNU General Public License along
140+ with this program. If not, see <http://www.gnu.org/licenses/>.
141+*/
142+
143+// Package whoopsie/identifier/testing implements a couple of Ids that
144+// are useful for testing things that use whoopsie/identifier.
145+package testing
146+
147+import "errors"
148+
149+// SettableIdentifier is an Id that lets you set the value of the identifier.
150+//
151+// By default the identifier's value is "<Settable>", so it's visible
152+// if you're misusing it.
153+type SettableIdentifier struct {
154+ value string
155+}
156+
157+// Settable is the constructor for SettableIdentifier.
158+func Settable() *SettableIdentifier {
159+ return &SettableIdentifier{"<Settable>"}
160+}
161+
162+// Set is the method you use to set the identifier.
163+func (sid *SettableIdentifier) Set(value string) {
164+ sid.value = value
165+}
166+
167+// Generate does nothing.
168+func (sid *SettableIdentifier) Generate() error {
169+ return nil
170+}
171+
172+// String returns the string you set.
173+func (sid *SettableIdentifier) String() string {
174+ return sid.value
175+}
176+
177+// FailingIdentifier is an Id that always fails to generate.
178+type FailingIdentifier struct{}
179+
180+// Failing is the constructor for FailingIdentifier.
181+func Failing() *FailingIdentifier {
182+ return &FailingIdentifier{}
183+}
184+
185+// Generate fails with an ubiquitous error.
186+func (*FailingIdentifier) Generate() error {
187+ return errors.New("lp0 on fire")
188+}
189+
190+// String returns "<Failing>".
191+//
192+// The purpose of this is to make it easy to spot if you're using it
193+// by accident.
194+func (*FailingIdentifier) String() string {
195+ return "<Failing>"
196+}
197
198=== added file 'whoopsie/identifier/testing/testing_test.go'
199--- whoopsie/identifier/testing/testing_test.go 1970-01-01 00:00:00 +0000
200+++ whoopsie/identifier/testing/testing_test.go 2014-01-15 13:48:09 +0000
201@@ -0,0 +1,70 @@
202+/*
203+ Copyright 2013-2014 Canonical Ltd.
204+
205+ This program is free software: you can redistribute it and/or modify it
206+ under the terms of the GNU General Public License version 3, as published
207+ by the Free Software Foundation.
208+
209+ This program is distributed in the hope that it will be useful, but
210+ WITHOUT ANY WARRANTY; without even the implied warranties of
211+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
212+ PURPOSE. See the GNU General Public License for more details.
213+
214+ You should have received a copy of the GNU General Public License along
215+ with this program. If not, see <http://www.gnu.org/licenses/>.
216+*/
217+
218+package testing
219+
220+import (
221+ identifier ".."
222+ . "launchpad.net/gocheck"
223+ "testing"
224+)
225+
226+// hook up gocheck
227+func Test(t *testing.T) { TestingT(t) }
228+
229+type IdentifierSuite struct{}
230+
231+var _ = Suite(&IdentifierSuite{})
232+
233+// TestSettableDefaultValueVisible tests SettableIdentifier's default
234+// value is notable.
235+func (s *IdentifierSuite) TestSettableDefaultValueVisible(c *C) {
236+ id := Settable()
237+ c.Check(id.String(), Equals, "<Settable>")
238+}
239+
240+// TestSettableSets tests SettableIdentifier is settable.
241+func (s *IdentifierSuite) TestSettableSets(c *C) {
242+ id := Settable()
243+ id.Set("hello")
244+ c.Check(id.String(), Equals, "hello")
245+}
246+
247+// TestSettableGenerateDoesNotFail tests SettableIdentifier's Generate
248+// does not fail.
249+func (s *IdentifierSuite) TestSettableGenerateDoesNotFail(c *C) {
250+ id := Settable()
251+ c.Check(id.Generate(), Equals, nil)
252+}
253+
254+// TestFailingFails tests FailingIdentifier fails.
255+func (s *IdentifierSuite) TestFailingFails(c *C) {
256+ id := Failing()
257+ c.Check(id.Generate(), Not(Equals), nil)
258+}
259+
260+// TestFailingStringNotEmpty tests FailingIdentifier still has a
261+// non-empty string.
262+func (s *IdentifierSuite) TestFailingStringNotEmpty(c *C) {
263+ id := Failing()
264+ c.Check(id.String(), Equals, "<Failing>")
265+}
266+
267+// TestIdentifierInterface tests FailingIdentifier and
268+// SettableIdentifier implement Id.
269+func (s *IdentifierSuite) TestIdentifierInterface(c *C) {
270+ _ = []identifier.Id{Failing(), Settable()}
271+}

Subscribers

People subscribed via source and target branches