Goblocks: an i3status replacement written in Go. Feedback welcome.

Hey guys,

I wrote an i3status replacement in Go called Goblocks. It uses the Go-i3barjson library to communicate with i3bar (which I posted about here a few weeks ago).

As with my previous post here, I'm looking to get feedback from more experienced Go developers. I'm fairly happy with the design overall, but the module system seems a little clumsy since there's a lot of duplicated boilerplate-type code. I have yet to come up with a satisfactory solution to that. Any feedback on that or any other aspect of Goblocks is much appreciated.

Thank you for your time.

---

评论：

nerr:

I would definitely recommend adding some tests. For example, have the update functions accept an io.Reader which is a file under normal operation, but could be a byte slice in tests. This way, you can validate that your code works as expected with a variety of input. 

This is very cool and I look forward to giving it a try!

Spirit_of_Stallman:

Gobocks requires Go version 1.7+

Wow. Why? :)

dhscholb:

1.7 is required because go-i3barjson uses the json SetIndent method for pretty printing the json encoder output, which was added in 1.7. I'm conflicted about having this requirement, because I like having the pretty printing feature, but I do realize that this will limit adoption in the short term.

saxykeyz:

reimplementing the setIndent method could in fact save the you the trouble of having to rely on go1.7

jrwren:

What trouble?

tgulacsi:

https://github.com/davidscholberg/goblocks/blob/master/goblocks.go#L14
log.Fatalf would be easier, and wouldn't exit with 0 return code.

https://github.com/davidscholberg/goblocks/blob/master/lib/modules/battery.go#L48
You open the file, but don't close it on error!
A "defer f.Close()" would be enough.

This is a recurring problem.

ratatask:

Your struct methods should probably take pointers as a receiver,
This

func (c Battery) GetBlockIndex() int {
    return c.BlockIndex
}

Copies the entire Battery struct unnecessarily.

dhscholb:

I thought that it was good practice to use pointer receivers only if the method needs to modify the struct, unless the object is too big. I guess my question is: how big is too big in this case?

ratatask:

I use pointer receivers almost exclusively, just to be consistent - though if the struct has a size similar to a couple of pointers, maybe it's not worth it.
See https://github.com/golang/go/wiki/CodeReviewComments#receiver-type if you want more guidelines.

progfrog:

Why is there block_index on each block in goblocks.yml? it shouldn't have to be mandatory; you can easily infer block position just by going through all blocks.
Or if you want to, you can assign block_index, but it shouldn't be mandatory.

dhscholb:

I agree that requiring block_index is a bit awkward. The reason I have the block_index field is because of the way the config is unmarshalled. If you look at the BlockConfigs struct in lib/modules/configure.go, each block is either a struct or a struct slice. I could, of course, define the block configs in a YAML array so that the index is inferred, but then I'm not sure how to unmarshal that into the distinct block config structs.

semi-:

I haven't looked at your code any, but just a suggestion: could you throw some screenshots up on your github? I mean I get what your project does and it seems relevant to my interests, I might even give it a try later, but as a lazy internetter it would be helpful to get a glimpse of what the end result can look like. 

dhscholb:

I do agree with this suggestion. I was initially hesitant to have a screenshot since the look of the end result is highly dependent on your i3bar and Goblocks config. Having said that, I do think it would be useful to have a screenshot that shows what some simple Goblocks config will look like, so I will get to that at some point.