SANDBOX-1282 | refactor: replace "wrapf" calls with stdlib wrap calls#1257
Conversation
There is no need to keep the legacy "wrapf" calls to wrap errors anymore, so the idea is to remove them in favor of the wrapping calls of the stdlib. SANDBOX-1282
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaced usages of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/templates/notificationtemplates/notification_generator.go (1)
63-63: Avoid wrapping a freshly-created static-string error with%w.
fmt.Errorf("unable to load templates: %w", errors.New("path must contain directory and file"))wraps a synthetic error whose only content is a literal string — unwrapping yields nothing a caller can meaningfully match on (no sentinel, no typed error). This is just a two-part message dressed up as a wrapped error, and it still keeps thegithub.com/pkg/errorsdependency alive in this file.Consider collapsing into a single
fmt.Errorf(no%wneeded) and dropping thepkg/errorsimport:♻️ Proposed change
- return nil, fmt.Errorf("unable to load templates: %w", errors.New("path must contain directory and file")) + return nil, fmt.Errorf("unable to load templates: path must contain directory and file")- return nil, fmt.Errorf("unable to load templates: %w", errors.New("must contain notification.html and subject.txt")) + return nil, fmt.Errorf("unable to load templates: must contain notification.html and subject.txt")Note:
errors.Wrapat Line 38 still usespkg/errors; migrating it would allow removing the import entirely, consistent with this PR's goal.Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/templates/notificationtemplates/notification_generator.go` at line 63, Replace the unnecessary wrapped static error in the fmt.Errorf call (currently using "%w" with errors.New("path must contain directory and file")) with a single fmt.Errorf message (e.g., fmt.Errorf("unable to load templates: path must contain directory and file")), and remove the now-unused github.com/pkg/errors import; also migrate any remaining uses of pkg/errors (notably the errors.Wrap call earlier in this file) to the stdlib fmt.Errorf or errors package so the pkg/errors dependency can be dropped entirely.pkg/templates/usertiers/usertier_generator.go (1)
36-42: Migration is incomplete in this file.Line 146 is correctly migrated, but several
pkg/errorscalls remain and keep the dependency alive:
- Line 36:
errors.Wrap(err, "unable to create UserTier generator")→fmt.Errorf("unable to create UserTier generator: %w", err)- Line 42:
errors.Wrap(err, "unable to create UserTiers")→fmt.Errorf("unable to create UserTiers: %w", err)- Line 155:
errors.Errorf("unable to load templates: unknown scope for file '%s'", name)→fmt.Errorf(...)(no wrapped error needed here)Given the PR’s stated intent to remove legacy
pkg/errorswrapping, consider migrating these too so the import can be dropped from this file.Also applies to: 146-146, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/templates/usertiers/usertier_generator.go` around lines 36 - 42, Replace remaining pkg/errors usage in this file: change errors.Wrap(err, "unable to create UserTier generator") and errors.Wrap(err, "unable to create UserTiers") to fmt.Errorf("unable to create UserTier generator: %w", err) and fmt.Errorf("unable to create UserTiers: %w", err) respectively, and replace errors.Errorf("unable to load templates: unknown scope for file '%s'", name) with fmt.Errorf("unable to load templates: unknown scope for file %q", name); update the imports by removing "github.com/pkg/errors" and adding "fmt" (or using the existing fmt import) so the package no longer depends on pkg/errors; ensure these changes are applied in the functions that call createUserTiers and the template-loading code paths (refer to createUserTiers and the template-loading error string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/templates/notificationtemplates/notification_generator.go`:
- Line 63: Replace the unnecessary wrapped static error in the fmt.Errorf call
(currently using "%w" with errors.New("path must contain directory and file"))
with a single fmt.Errorf message (e.g., fmt.Errorf("unable to load templates:
path must contain directory and file")), and remove the now-unused
github.com/pkg/errors import; also migrate any remaining uses of pkg/errors
(notably the errors.Wrap call earlier in this file) to the stdlib fmt.Errorf or
errors package so the pkg/errors dependency can be dropped entirely.
In `@pkg/templates/usertiers/usertier_generator.go`:
- Around line 36-42: Replace remaining pkg/errors usage in this file: change
errors.Wrap(err, "unable to create UserTier generator") and errors.Wrap(err,
"unable to create UserTiers") to fmt.Errorf("unable to create UserTier
generator: %w", err) and fmt.Errorf("unable to create UserTiers: %w", err)
respectively, and replace errors.Errorf("unable to load templates: unknown scope
for file '%s'", name) with fmt.Errorf("unable to load templates: unknown scope
for file %q", name); update the imports by removing "github.com/pkg/errors" and
adding "fmt" (or using the existing fmt import) so the package no longer depends
on pkg/errors; ensure these changes are applied in the functions that call
createUserTiers and the template-loading code paths (refer to createUserTiers
and the template-loading error string).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 405ec1c8-1a41-4aca-8ec6-32ec0613bba2
📒 Files selected for processing (4)
cmd/main.gocontrollers/usersignup/approval.gopkg/templates/notificationtemplates/notification_generator.gopkg/templates/usertiers/usertier_generator.go
metlos
left a comment
metlos
left a comment
There was a problem hiding this comment.
Yay for using the stdlib :) Just minor comments below that don't block the merge IMHO.
| segments := strings.Split(path, "/") | ||
| if len(segments) != 5 { | ||
| return nil, errors.Wrapf(errors.New("path must contain directory and file"), "unable to load templates") | ||
| return nil, fmt.Errorf("unable to load templates: %w", errors.New("path must contain directory and file")) |
There was a problem hiding this comment.
Agreed with coderabbit here. errors.New doesn't make sense here. I'd just use fmt.Errorf with just the single string. Alternatively, you can use errors.New with that string or even a global private error var with this error.
There was a problem hiding this comment.
Yeah, I went with a simple errors.New because we don't need to errors.As or errors.Is it
| notificationTemplates[directoryName] = template | ||
| default: | ||
| return nil, errors.Wrapf(errors.New("must contain notification.html and subject.txt"), "unable to load templates") | ||
| return nil, fmt.Errorf("unable to load templates: %w", errors.New("must contain notification.html and subject.txt")) |
There was a problem hiding this comment.
Makes sense, done!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, metlos, mfrancisc, MikelAlejoBR The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
MikelAlejoBR
merged commit 251ea71
into
codeready-toolchain:master
4 participants
There is no need to keep the legacy "wrapf" calls to wrap errors anymore, so the idea is to remove them in favor of the wrapping calls of the stdlib.
Related PRs
Jira ticket
[SANDBOX-1282]
Summary by CodeRabbit