Skip to content
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

change type of Level to int #141

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

imjasonh
Copy link
Contributor

@imjasonh imjasonh commented Aug 21, 2024

Marking this as draft to gather feedback

The Level type (int32) is currently slightly incompatible with stdlib's slog.Level type, which is an int. This means it's impossible to safely cast a slog.Level to a charmlog.Level, without risking an overflow. (Practically speaking, that's very unlikely, but try telling that to the linter 😆).

This PR changes type Level int32 to type Level int and updates some places where atomic.StoreInt32 was called to atomic.StoreInt64.

It's entirely possible that this change isn't safe, and that there's already enough code out there that expects charmlog.Level to be an int32 that 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.

Signed-off-by: Jason Hall <jason@chainguard.dev>
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.63%. Comparing base (2338a13) to head (242f811).
Report is 27 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jason Hall <jason@chainguard.dev>
@imjasonh
Copy link
Contributor Author

imjasonh commented Sep 6, 2024

Gentle ping. 🙏

@aymanbagabas
Copy link
Member

Hi @imjasonh, thank you for sending this patch. I think this change is safe and reasonable. I'll merge once the tests pass

@aymanbagabas aymanbagabas marked this pull request as ready for review September 9, 2024 13:52
@aymanbagabas aymanbagabas self-requested a review as a code owner September 9, 2024 13:52
@imjasonh
Copy link
Contributor Author

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?

@aymanbagabas
Copy link
Member

Can I send a separate PR to disable that check for 1.19? Or install govulncheck instead of building it?

That would be lovely!

@imjasonh
Copy link
Contributor Author

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.

@imjasonh
Copy link
Contributor Author

Hey @aymanbagabas, charmbracelet/meta#162 is merged and this PR is green! 🕺

@aymanbagabas aymanbagabas merged commit f954dc8 into charmbracelet:main Sep 11, 2024
13 checks passed
@aymanbagabas
Copy link
Member

Thank you @imjasonh!

1 similar comment
@aymanbagabas
Copy link
Member

Thank you @imjasonh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants