Readability feedback

[stapelberg]

As discussed in person, here are a bunch of thoughts regarding readability improvements from a quick glance at the code:

• A bunch of places in the code panic. They should either return an error, or — typically only for testing — follow the must* naming convention, e.g. mustOpenHostMemory. The same holds for log.Fatal, which also terminates the process, which is probably not desirable, especially as the code grows to fulfill more functions.
• The syscall package in the standard library is frozen in favor of the golang.org/x/sys/unix. While you might not have a specific reason to use the x/sys/unix package yet (or maybe some constants you’re currently defining are to be found there?), it might be good style to start using it for consistency as the code grows.
• Currently, flags are defined all over the place. Ideally, flags would only be defined in the main package, which should pass the flag values to the rest of the code. Otherwise, flags will be a hindrance to testing, might conflict, etc.
• optionally, you might consider replacing some of the Makefiles with “go generate” instructions
• For testing, it’s not idiomatic to pass *testing.T around. Instead, compare actual and expected state, return a diff, and call t.Errorf (or t.Fatalf) in your test if the diff is non-empty.

This list might not be exhaustive, but that’s what I could find right now :)

Hope that helps!

[bluecmd]

Hi @stapelberg.

This is golden, thank you so much!

My plan right now is to reach a point where I have a platform with the base functionality (like, I don't know, being able to actually power on the damn thing :p) and then I'll take out the big broom and think hard and long of the structures. I'll definitely use these tips for then, and if you have more do let me know.

[bluecmd]

We discussed over hangouts that I should add a platform_test.go for the platform GPIO lib that catches any initialization errors with the GPIO mapping.

[bluecmd]

@mdlayher hinted that https://godoc.org/golang.org/x/sync/errgroup might be good to switch to as well.