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

Implement PluginErrorf #1228

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Implement PluginErrorf #1228

merged 1 commit into from
Feb 13, 2025

Conversation

fabrizio-grafana
Copy link
Contributor

@fabrizio-grafana fabrizio-grafana commented Feb 11, 2025

What this PR does / why we need it:
This PR implements the PluginErrorf function, which works similarly to DownstreamErrorf.

@fabrizio-grafana fabrizio-grafana requested a review from a team as a code owner February 11, 2025 13:00
@fabrizio-grafana fabrizio-grafana requested review from andresmgot, oshirohugo and s4kh and removed request for a team February 11, 2025 13:00
@CLAassistant
Copy link

CLAassistant commented Feb 11, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Why is this needed? I personally find this a little redundant considering we can do PluginError(fmt.Errorf(format, a...)). So I am wondering why do we need it.

@fabrizio-grafana
Copy link
Contributor Author

fabrizio-grafana commented Feb 11, 2025

Wouldn't it be the same for DownstreamErrorf then? I'm fine in not having PluginErrorf, but to me it seems more confusing to only have DownstreamErrorf without the plugin error counterpart than having both DownstreamErrorf and PluginErrorf.

@ivanahuckova
Copy link
Member

DownstreamErrorf is not really used anywhere and I would rather get rid of that as I personally think it is unnecessary wrapper. But that would be potentially breaking change - so not planning to do that. But also, if we want to add PluginErrorf, I am wondering what is the reason, so we are not unnecessarily adding more code.

@fabrizio-grafana
Copy link
Contributor Author

fabrizio-grafana commented Feb 11, 2025

I am wondering what is the reason

One of your plugin wraps errors raised by third-party libraries with custom error messages, so both PluginErrorf and DownstreamErrorf can be useful. But I'm fine in either finding a different approach or using PluginError(fmt.Errorf(...))), and similarly for downstream errors.

I would rather get rid of [DownstreamErrorf] [...] But that would be potentially breaking change

Do you think we can at least mark it as deprecated? Proposal here.
I'm fine in not removing it now, since it is a breaking change. But I'm strongly opinionated in favor of marking it as deprecated at least. Or, as I said, it might be confusing to only have DownstreamErrorf without PluginErrorf.

@andresmgot
Copy link
Contributor

My two cents is that you don't need to deprecate de function if we now it's not being used, you can directly delete it (it's an internal function). In any case, if you want to keep DownstreamErrorf and PluginErrorf, I don't see the problem either (having this syntax sugar to avoid PluginError(fmt.Errorf(... if it's a common thing).

@fabrizio-grafana
Copy link
Contributor Author

it's not being used

Apparently it is used, even though in just a couple of places.

if you want to keep DownstreamErrorf and PluginErrorf, I don't see the problem either

Good! OK maybe I'll wait to merge or close the PR to see if someone else wants to express their opinions, since I'm not sure who should take the final decision here—I can, but I'm not an owner of the library here, so I'm probably not the best person.

@fabrizio-grafana fabrizio-grafana merged commit a29bee2 into main Feb 13, 2025
3 checks passed
@fabrizio-grafana fabrizio-grafana deleted the add-PluginErrorf branch February 13, 2025 14:10
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.

4 participants