-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
scd4x: SCD4x CO2 Sensor - Initial Add #81
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
=======================================
+ Coverage 47.5% 48.0% +0.5%
=======================================
Files 87 88 +1
Lines 10935 11251 +316
=======================================
+ Hits 5197 5401 +204
- Misses 5542 5620 +78
- Partials 196 230 +34 ☔ View full report in Codecov by Sentry. |
Thanks! Will look later. |
@@ -0,0 +1,56 @@ | |||
# Sensirion SCD4x CO<sub>2</sub> Sensors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason to not put this into doc.go? I would expect to read the doc from pkg.go.dev, so I would think it would be more discoverable by being there.
@@ -0,0 +1,63 @@ | |||
//go:build examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove, unnecessary.
|
||
// basic example program for scd4x sensors using this library. | ||
// | ||
// To execute this as a stand-alone program: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will already be presented correctly on pkg.go.dev, this is unnecessary. Otherwise we'd have to put this on every single examples?
|
||
// Return the sensor readings in string format. | ||
func (e *Env) String() string { | ||
return fmt.Sprintf("Temperature: %s Humidity: %s CO2: %s", e.Temperature.String(), e.Humidity.String(), e.CO2.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, .String() call are unnecessary when using %s, it will be called automatically. See point "5." at https://pkg.go.dev/fmt.
{Addr: SensorAddress, W: []uint8{0x36, 0xf6}}, | ||
{Addr: SensorAddress, W: []uint8{0x21, 0xb1}}} | ||
|
||
func init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way would be to use TestMain as described at https://pkg.go.dev/testing#hdr-Main.
func TestMain(m *testing.M) {
flag.BoolVar(&liveDevice, "live", os.Getenv("SCD4X")!="", "Run unit test on a live device (a smoke test). Set automatically if env var SCD4X is set")
m.Run()
}
Generally I've put these into cmd smoketest but I don't mind much, this is fine. |
None of the comments are blocking so I'll merge right away, you can do a follow up if you want but it's not required. |
@maruel I'm mostly liking they way I did the unit tests. I get live device, and playback testing without lots of extra code.
What are your thoughts? Do you have any ideas on making it more elegant?