change type of Level to int#141
Conversation
Signed-off-by: Jason Hall <jason@chainguard.dev>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 80.92% 82.63% +1.71%
==========================================
Files 11 11
Lines 739 714 -25
==========================================
- Hits 598 590 -8
+ Misses 126 108 -18
- Partials 15 16 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jason Hall <jason@chainguard.dev>
|
Gentle ping. 🙏 |
|
Hi @imjasonh, thank you for sending this patch. I think this change is safe and reasonable. I'll merge once the tests pass |
Thanks! The test failure seems unrelated to my change, it seems govulncheck doesn't build with Go 1.19 anymore. Can I send a separate PR to disable that check for 1.19? Or install govulncheck instead of building it? |
That would be lovely! |
|
charmbracelet/meta#162 attempts to solve the govulncheck-vs-Go-1.19 issue, let me know if that looks good and we can see if it unblocks this PR. |
|
Hey @aymanbagabas, charmbracelet/meta#162 is merged and this PR is green! 🕺 |
|
Thank you @imjasonh! |
1 similar comment
|
Thank you @imjasonh! |
Marking this as draft to gather feedback
The
Leveltype (int32) is currently slightly incompatible with stdlib'sslog.Leveltype, which is anint. This means it's impossible to safely cast aslog.Levelto acharmlog.Level, without risking an overflow. (Practically speaking, that's very unlikely, but try telling that to the linter 😆).This PR changes
type Level int32totype Level intand updates some places whereatomic.StoreInt32was called toatomic.StoreInt64.It's entirely possible that this change isn't safe, and that there's already enough code out there that expects
charmlog.Levelto be anint32that this would break enough folks not to be worth it.But, if this change is doable, it would make it slightly easier for code that uses this package to interoperate with stdlib's
slog.