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

chore: prefix log level consts, minor package updates #39

Merged
merged 3 commits into from
Nov 19, 2023
Merged

Conversation

nilslice
Copy link
Member

While using the SDK, I found it difficult to use the log level consts and to assign a log level in the plugin configuration struct.

This changes the *LogLevel field to use a non-pointer type, and updates the code accordingly. I think this is ok, since we've define the iota 0 value to a Off constant. This will be the default / zero-value of the type when left unset in a struct.

Also, I have prefixed these log level const names with LogLevel -- I think it makes it more clear when looking at the name, e.g. extism.LogLevelInfo vs. extism.Info

Additionally, although unrelated, this PR includes a couple minor changes to use non-deprecated functions from io & os packages vs. ioutil.

@nilslice
Copy link
Member Author

Also just realized the change regarding the Off and the pointer value in the config struct has an unintended side effect of not knowing if the level was unset by the end user vs. Off by default.

We could make the starting iota zero value something like LogLevelUnset so that in the default case we have something there. It's not ideal, but I still prefer it to having to declare a variable out of line of the struct definition and take a reference to it.

extism.go Outdated Show resolved Hide resolved
@nilslice
Copy link
Member Author

@mhmd-azeez -- I am not sure why the latest commit breaks tests. Would you mind picking this up? I am traveling today and will be offline for a bit. Ultimately, I'd like to preserve the ergonomics of using the non-pointer value, without cluttering the public exported consts.

@mhmd-azeez
Copy link
Collaborator

@nilslice sure

Copy link
Collaborator

@mhmd-azeez mhmd-azeez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nilslice I love the unexported logLevelUnset solution, the tests are now working too

@mhmd-azeez mhmd-azeez merged commit c4e57ce into main Nov 19, 2023
3 checks passed
@mhmd-azeez mhmd-azeez deleted the log-level branch November 19, 2023 14:25
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.

2 participants