Skip to content

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Jul 30, 2025

  • LogLevel has a String method to convert the numeric level into its string version
    • This also allows the compiler to automatically call the String method when formatting the value (fmt.Print).
  • There is only one name used to identify the default log level, and it is declared as a const DefaultName
    • Eliminates confusion over whether to use "", "*", "default", etc.
    • The value of the DefaultName const can change with requiring any change is code that uses the const.
  • Removed the "Get" name for functions that lookup a value.
    • "it's neither idiomatic nor necessary to put Get into the getter's name" see Effective Go
  • A DefaultLevel function returns the current log level and handles locking.
    • Code does need to worry about whether locking is needed to get the default, let the function do whatever is needed.
  • Function to convert string to LogLevel is named Parse
    • This is typical among other golang APIs when converting a string to another type.

- LogLevel has a String method to convert the numeric level into its string version
- There is only one name used to identify the default log level, and it is declared as a const `DefaultName`
- Removed the "Get" name for functions that lookup a value.
- A `DefaultLevel` function returns the current log level and handles locking.
- Function to convert string to LogLevel is named `Parse`
@gammazero gammazero requested a review from SgtPooki July 30, 2025 17:49
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

Makes sense. we’ll need to change kubo to address the new default name because i think there are expectations there of “*”.

Default name being an empty string or “*” make the same sense to me

const DefaultName = ""

// String returns the name of a LogLevel.
func (lvl LogLevel) String() string {
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@gammazero gammazero merged commit cbb68a7 into master Jul 30, 2025
9 checks passed
@gammazero gammazero deleted the loglevel-api branch July 30, 2025 23:06
@rvagg
Copy link
Member

rvagg commented Aug 20, 2025

This is really hurting downstream compatibility leading to the usual wac-a-mole of getting dependencies to work together (this being one of the dependencies that's used almost everywhere in our little ecosystem). Since it was a breaking change this would have been a good reason to /v3. In lieu of that, can we have these back please as aliases marked as deprecated?

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.

3 participants