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

Strict ParseLevel #79

Closed
dezren39 opened this issue Oct 20, 2023 · 1 comment · Fixed by #83
Closed

Strict ParseLevel #79

dezren39 opened this issue Oct 20, 2023 · 1 comment · Fixed by #83

Comments

@dezren39
Copy link
Contributor

hi! I like the default ParseLevel as it is now, but I have a problem. I have a situation where I need to ensure the level really exists, and so I need to not have a default level or for err to be returned too so i can check. guessing errors as values would be adding err to return type

https://github.com/charmbracelet/log/blob/v0.2.5/level.go#L42

I just really don't want to have my own switch case just to check, but I can't pass anything in and get INFO back in this case I need to know if it's a valid parse.

(actually I really like this default and rely on it already in other places.)

alternatives i came up with in the past couple minutes: (i can maybe make a pr idk)

  • create ParseLevelStrict() (ParseLevel would use parseLevelStrict else default INFO)
  • Change return signature to level, err and if default also return an err
  • an 'invalid/unknown' enum type (i do not like this)
  • change param signature, add default and strict params
    • ParseLevel(level string, default LogLevel, strict bool)

overall i think just adding ,err to return type would be best, maybe allowing default to be overridden but if the err is returned the caller can add their own default because the information isn't lost.

@aymanbagabas
Copy link
Member

Hi @dezren39, you have a good point here, ParseLevel shouldn't always assume InfoLevel when the level is invalid. Changing the signature to return an error makes a lot of sense. PRs are welcome 🙂

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 a pull request may close this issue.

2 participants