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

feat: replace deprecated logger methods #546

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

juev
Copy link
Contributor

@juev juev commented Dec 18, 2024

Overview

Fixed: #524

What this PR does / why we need it

Just replace deprecated logger methods

Special notes for your reviewer

nope

@juev juev added this to the 1.3.11 milestone Dec 18, 2024
@juev juev self-assigned this Dec 18, 2024
@juev juev added enhancement New feature or request go Pull requests that update Go code labels Dec 19, 2024
@juev juev force-pushed the feature/replace-deprecated-logger-methods branch from 35c8edb to 8214cc0 Compare December 19, 2024 05:51
@juev juev force-pushed the feature/replace-deprecated-logger-methods branch from 8214cc0 to 0881a1f Compare December 19, 2024 08:50
@juev juev marked this pull request as ready for review December 19, 2024 08:55
Comment on lines +198 to +201
if hookResult == nil {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What for? As I see, we only return nil when there is an error in the Execute() call
And even if we get nil, shouldn't we return err then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A rather strange logic is used here,

  1. We calculate the result hookResult, err := h.Execute(h.GetConfigVersion(), bc, "global", prefixedConfigValues, prefixedValues, logLabels)
  2. Next, we check that the result is not null and there is a Usage to display the result.
  3. Only after that we check for the error, and if there is one, return it.
  4. And after that, we are already trying to fully access the hookResult fields, although theoretically it is possible that we get nil. As a result, instead of a mistake, we will get panic.
  5. To avoid panic, we do a check on nil before accessing the hookResult fields. Since we've already checked for errors and they're no longer there, since we've got a hookResult with nil, we'll just exit the function with the result nil.

In a good way, after the calculation, you need to immediately check the error, if there is one, return it. Then check for nil, and then decide what to do, return a new error or return nil. And then process the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the logic is strange, or even tightly coupled with what h.Execute() returns, but if there is no possibility of getting nil without errors, does it make sense to check? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

and if we check and get nil for the result, but w/o an error, it looks to me like an error condition 🤔 . I think we'd better return a custom error like 'something went wrong' instead of concealing the fact that we'd got nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to think about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[refactoring] Replace all deprecated logger methods
2 participants