Merge lp:~chipaca/ubuntu-push/whoopsie-identifier into lp:ubuntu-push

[John Lenton]

A thin wrapper around libwhoopsie/identifier.h.

A thin wrapper around libwhoopsie/identifier.h.

[Samuele Pedroni (pedronis)]

* 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

[John Lenton (chipaca)]

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?

[lp:~chipaca/ubuntu-push/whoopsie-identifier]

44. By John Lenton on 2014-01-15

merged trunk

45. By John Lenton on 2014-01-15

clean up and addressed issues raised by Samuele's review

46. By John Lenton on 2014-01-15

docs

[Samuele Pedroni (pedronis)]

looks good but needs a make format pass

review: Approve

[lp:~chipaca/ubuntu-push/whoopsie-identifier]

47. By John Lenton on 2014-01-15

make format

=== added directory 'whoopsie'
=== added directory 'whoopsie/identifier'
=== added file 'whoopsie/identifier/identifier.go'
--- whoopsie/identifier/identifier.go	1970-01-01 00:00:00 +0000
+++ whoopsie/identifier/identifier.go	2014-01-15 13:48:09 +0000
@@ -0,0 +1,66 @@
+/*
+ Copyright 2013-2014 Canonical Ltd.
+
+ This program is free software: you can redistribute it and/or modify it
+ under the terms of the GNU General Public License version 3, as published
+ by the Free Software Foundation.
+
+ This program is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranties of
+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ PURPOSE.  See the GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+// Package whoopsie/identifier wraps libwhoopsie, and is thus the
+// source of an anonymous and stable system id used by the Ubuntu
+// error tracker and the Ubuntu push notifications service.
+package identifier
+
+/*
+#cgo pkg-config: libwhoopsie
+#include <glib.h>
+#include <libwhoopsie/identifier.h>
+*/
+import "C"
+import "unsafe"
+import "errors"
+
+// an Id knows how to generate itself, and how to stringify itself.
+type Id interface {
+	Generate() error
+	String() string
+}
+
+// Identifier is the default Id implementation.
+type Identifier struct {
+	value string
+}
+
+// New creates an Identifier, but does not call Generate() on it.
+func New() *Identifier {
+	return &Identifier{}
+}
+
+// Generate makes the Identifier create the identifier itself.
+func (id *Identifier) Generate() error {
+	var gerr *C.GError
+	var cs *C.char
+	defer C.g_free((C.gpointer)(unsafe.Pointer(cs)))
+	C.whoopsie_identifier_generate(&cs, &gerr)
+
+	if gerr != nil {
+		return errors.New(C.GoString((*C.char)(gerr.message)))
+	} else {
+		id.value = C.GoString(cs)
+		return nil
+	}
+
+}
+
+// String returns the system identifier as a string.
+func (id *Identifier) String() string {
+	return id.value
+}
 
=== added file 'whoopsie/identifier/identifier_test.go'
--- whoopsie/identifier/identifier_test.go	1970-01-01 00:00:00 +0000
+++ whoopsie/identifier/identifier_test.go	2014-01-15 13:48:09 +0000
@@ -0,0 +1,43 @@
+/*
+ Copyright 2013-2014 Canonical Ltd.
+
+ This program is free software: you can redistribute it and/or modify it
+ under the terms of the GNU General Public License version 3, as published
+ by the Free Software Foundation.
+
+ This program is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranties of
+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ PURPOSE.  See the GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package identifier
+
+import (
+	. "launchpad.net/gocheck"
+	"testing"
+)
+
+// hook up gocheck
+func Test(t *testing.T) { TestingT(t) }
+
+type IdentifierSuite struct{}
+
+var _ = Suite(&IdentifierSuite{})
+
+// TestGenerate checks that Generate() does not fail, and returns a
+// 128-byte string.
+func (s *IdentifierSuite) TestGenerate(c *C) {
+	id := New()
+
+	c.Check(id.Generate(), Equals, nil)
+	c.Check(id.String(), HasLen, 128)
+}
+
+// TestIdentifierInterface checks that Identifier implements Id.
+func (s *IdentifierSuite) TestIdentifierInterface(c *C) {
+	_ = []Id{New()}
+}
 
=== added directory 'whoopsie/identifier/testing'
=== added file 'whoopsie/identifier/testing/testing.go'
--- whoopsie/identifier/testing/testing.go	1970-01-01 00:00:00 +0000
+++ whoopsie/identifier/testing/testing.go	2014-01-15 13:48:09 +0000
@@ -0,0 +1,70 @@
+/*
+ Copyright 2013-2014 Canonical Ltd.
+
+ This program is free software: you can redistribute it and/or modify it
+ under the terms of the GNU General Public License version 3, as published
+ by the Free Software Foundation.
+
+ This program is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranties of
+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ PURPOSE.  See the GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+// Package whoopsie/identifier/testing implements a couple of Ids that
+// are useful for testing things that use whoopsie/identifier.
+package testing
+
+import "errors"
+
+// SettableIdentifier is an Id that lets you set the value of the identifier.
+//
+// By default the identifier's value is "<Settable>", so it's visible
+// if you're misusing it.
+type SettableIdentifier struct {
+	value string
+}
+
+// Settable is the constructor for SettableIdentifier.
+func Settable() *SettableIdentifier {
+	return &SettableIdentifier{"<Settable>"}
+}
+
+// Set is the method you use to set the identifier.
+func (sid *SettableIdentifier) Set(value string) {
+	sid.value = value
+}
+
+// Generate does nothing.
+func (sid *SettableIdentifier) Generate() error {
+	return nil
+}
+
+// String returns the string you set.
+func (sid *SettableIdentifier) String() string {
+	return sid.value
+}
+
+// FailingIdentifier is an Id that always fails to generate.
+type FailingIdentifier struct{}
+
+// Failing is the constructor for FailingIdentifier.
+func Failing() *FailingIdentifier {
+	return &FailingIdentifier{}
+}
+
+// Generate fails with an ubiquitous error.
+func (*FailingIdentifier) Generate() error {
+	return errors.New("lp0 on fire")
+}
+
+// String returns "<Failing>".
+//
+// The purpose of this is to make it easy to spot if you're using it
+// by accident.
+func (*FailingIdentifier) String() string {
+	return "<Failing>"
+}
 
=== added file 'whoopsie/identifier/testing/testing_test.go'
--- whoopsie/identifier/testing/testing_test.go	1970-01-01 00:00:00 +0000
+++ whoopsie/identifier/testing/testing_test.go	2014-01-15 13:48:09 +0000
@@ -0,0 +1,70 @@
+/*
+ Copyright 2013-2014 Canonical Ltd.
+
+ This program is free software: you can redistribute it and/or modify it
+ under the terms of the GNU General Public License version 3, as published
+ by the Free Software Foundation.
+
+ This program is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranties of
+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ PURPOSE.  See the GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package testing
+
+import (
+	identifier ".."
+	. "launchpad.net/gocheck"
+	"testing"
+)
+
+// hook up gocheck
+func Test(t *testing.T) { TestingT(t) }
+
+type IdentifierSuite struct{}
+
+var _ = Suite(&IdentifierSuite{})
+
+// TestSettableDefaultValueVisible tests SettableIdentifier's default
+// value is notable.
+func (s *IdentifierSuite) TestSettableDefaultValueVisible(c *C) {
+	id := Settable()
+	c.Check(id.String(), Equals, "<Settable>")
+}
+
+// TestSettableSets tests SettableIdentifier is settable.
+func (s *IdentifierSuite) TestSettableSets(c *C) {
+	id := Settable()
+	id.Set("hello")
+	c.Check(id.String(), Equals, "hello")
+}
+
+// TestSettableGenerateDoesNotFail tests SettableIdentifier's Generate
+// does not fail.
+func (s *IdentifierSuite) TestSettableGenerateDoesNotFail(c *C) {
+	id := Settable()
+	c.Check(id.Generate(), Equals, nil)
+}
+
+// TestFailingFails tests FailingIdentifier fails.
+func (s *IdentifierSuite) TestFailingFails(c *C) {
+	id := Failing()
+	c.Check(id.Generate(), Not(Equals), nil)
+}
+
+// TestFailingStringNotEmpty tests FailingIdentifier still has a
+// non-empty string.
+func (s *IdentifierSuite) TestFailingStringNotEmpty(c *C) {
+	id := Failing()
+	c.Check(id.String(), Equals, "<Failing>")
+}
+
+// TestIdentifierInterface tests FailingIdentifier and
+// SettableIdentifier implement Id.
+func (s *IdentifierSuite) TestIdentifierInterface(c *C) {
+	_ = []identifier.Id{Failing(), Settable()}
+}