Merge pull request #374 from travisturner/todo-fixes

tidy up some of the TODO comments

client.go

@@ -44,6 +44,8 @@ type FieldValue struct {
 // something hasn't been architected correctly.
 // While I understand that putting the entire Client behind an interface might require this many methods,
 // I don't want to let it go unquestioned.
+// Another note from Travis: I think we eventually want to unify `InternalClient` with the `go-pilosa` client.
+// Doing that may obviate the need to refactor this.
 type InternalClient interface {
 	InternalQueryClient
 

cmd/root.go

@@ -31,13 +31,13 @@ func NewRootCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 		productName = "Pilosa Enterprise " + pilosa.Version
 	}
 	rc := &cobra.Command{
-		Use:   "pilosa",
+		Use: "pilosa",
+		// TODO: These short/long descriptions could use some updating.
 		Short: "Pilosa - A Distributed In-memory Binary Bitmap Index.",
-		// TODO - is documentation actually there?
 		Long: `Pilosa is a fast index to turbocharge your database.
 
 This binary contains Pilosa itself, as well as common
-tools for administering pilosa, importing/exporting data,
+tools for administering Pilosa, importing/exporting data,
 backing up, and more. Complete documentation is available
 at https://www.pilosa.com/docs/.
 

encoding/proto/proto.go

@@ -1253,13 +1253,12 @@ func decodeTransactionMessage(pb *internal.TransactionMessage, m *pilosa.Transac
 }
 
 func decodeTransaction(pb *internal.Transaction, trns *pilosa.Transaction) {
-
 	trns.ID = pb.ID
 	trns.Active = pb.Active
 	trns.Exclusive = pb.Exclusive
 	trns.Timeout = time.Duration(pb.Timeout)
 	trns.Deadline = time.Unix(0, pb.Deadline)
-	// TODO m.Stats... once it has anything
+	// TODO: trns.Stats... once it has anything
 }
 
 // QueryResult types.

handler.go

@@ -57,7 +57,9 @@ type QueryRequest struct {
 // QueryResponse represent a response from a processed query.
 type QueryResponse struct {
 	// Result for each top-level query call.
-	// Can be a Bitmap, Pairs, or uint64. // TODO: this comment is out of date.
+	// The result type differs depending on the query; types
+	// include: Row, RowIdentifiers, GroupCounts, SignedRow,
+	// ValCount, Pair, Pairs, bool, uint64.
 	Results []interface{}
 
 	// Set of column attribute objects matching IDs returned in Result.

logger/filewriter_test.go

@@ -43,6 +43,8 @@ import (
 //
 func TestReopenAppend(t *testing.T) {
 	// TODO fix
+	// (travis) I have no idea what this TODO is asking for.
+	// Perhaps use `ioutil.TempFile()`?
 	var fname = "/tmp/foo"
 
 	// Step 1 -- Create a sample file using normal means
@@ -105,6 +107,8 @@ func TestReopenAppend(t *testing.T) {
 //
 func TestChangeInode(t *testing.T) {
 	// TODO fix
+	// (travis) I have no idea what this TODO is asking for.
+	// Perhaps use `ioutil.TempFile()`?
 	var fname = "/tmp/foo"
 
 	// Step 1 -- Create a empty sample file

mock/translator.go

@@ -81,12 +81,10 @@ func (s *TranslateStore) EntryReader(ctx context.Context, offset uint64) (pilosa
 	return s.EntryReaderFunc(ctx, offset)
 }
 
-// TODO: implement this
 func (s *TranslateStore) WriteTo(w io.Writer) (int64, error) {
 	return 0, nil
 }
 
-// TODO: implement this
 func (s *TranslateStore) ReadFrom(r io.Reader) (int64, error) {
 	return 0, nil
 }

server.go

@@ -88,7 +88,6 @@ type Server struct { // nolint: maligned
 }
 
 // Holder returns the holder for server.
-// TODO: have this return an interface for Holder instead of concrete object?
 func (s *Server) Holder() *Holder {
 	return s.holder
 }
@@ -1159,7 +1158,6 @@ func countOpenFiles() (int, error) {
 		lines := strings.Split(string(out), strconv.Itoa(pid))
 		return len(lines), nil
 	case "windows":
-		// TODO: count open file handles on windows
 		return 0, errors.New("countOpenFiles() on Windows is not supported")
 	default:
 		return 0, errors.New("countOpenFiles() on this OS is not supported")

test/pilosa.go

@@ -298,6 +298,9 @@ func (c Cluster) ImportBits(t testing.TB, index, field string, rowcols [][2]uint
 			t.Fatalf("getting shard nodes: %v", err)
 		}
 		// TODO won't be necessary to do all nodes once that works hits
+		// (travis) this TODO is not clear to me, but I think it's
+		// suggesting that elsewhere we would support importing to a
+		// single node, regardless of where the data ends up.
 		for _, node := range nodes {
 			for _, com := range c {
 				if com.API.Node().ID != node.ID {

transaction.go

@@ -42,8 +42,9 @@ type Transaction struct {
 	// Timeout is the minimum idle time for which this transaction should continue to exist.
 	Timeout time.Duration `json:"timeout"`
 
-	// Deadline is calculated from Timeout. TODO reset deadline each time there is activity on the transaction. (we can't do this until there is some method of associating a request/call with a transaction)
-	// time there is activity on the transaction.
+	// Deadline is calculated from Timeout. TODO reset deadline each time there is activity
+	// on the transaction. (we can't do this until there is some method of associating a
+	// request/call with a transaction)
 	Deadline time.Time `json:"deadline"`
 
 	// Stats track statistics for the transaction. Not yet used.

translate.go

@@ -414,16 +414,20 @@ func (s *InMemTranslateStore) EntryReader(ctx context.Context, offset uint64) (T
 	return newInMemTranslateEntryReader(ctx, s, offset), nil
 }
 
-// TODO: implement this
-// WriteTo writes the contents of the store to the writer.
+// WriteTo ensures that the TranslateStore implements io.WriterTo.
+// It's not important that this be implemented. It would really
+// only be necessary if we wanted to test cluster resizing while using
+// an in-memory translate store.
 func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) {
-	return 0, nil
+	return 0, nil // TODO: try to use ErrNotImplemented
 }
 
-// TODO: implement this
 // ReadFrom ensures that the TranslateStore implements io.ReaderFrom.
+// It's not important that this be implemented. It would really
+// only be necessary if we wanted to test cluster resizing while using
+// an in-memory translate store.
 func (s *InMemTranslateStore) ReadFrom(r io.Reader) (int64, error) {
-	return 0, nil
+	return 0, nil // TODO: try to use ErrNotImplemented
 }
 
 // MaxID returns the highest identifier in the store.