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

Unify Callback Function Naming #966

Closed
MrAlias opened this issue Jul 24, 2020 · 3 comments · Fixed by #1061
Closed

Unify Callback Function Naming #966

MrAlias opened this issue Jul 24, 2020 · 3 comments · Fixed by #1061
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jul 24, 2020

From recent feedback:

Currently, there isn’t a convention in naming for callback-like functions. GetHookFunc, BatchObserverCallback. This gives users an impression there is a specific reason they are named differently and may have differences in behavior although they don’t.
We can pick one for consistency and developer productivity. *Callback is used less in Go, *Func or *Handler would be a good fit.

@MrAlias MrAlias added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers release:required-for-ga labels Jul 24, 2020
@dengliming
Copy link
Member

@MrAlias Hello, I'd love to take this as first contribution for this repo. I suggest rename *Callback with *Handler. WDYT?

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 15, 2020

Thanks for the help! It's much appreciated 🎉. I'll assign this issue to you.

@MrAlias Hello, I'd love to take this as first contribution for this repo. I suggest rename *Callback with *Handler. WDYT?

I'd prefer to go with *Func instead of *Handler. This is based on the fact that we have an ErrorHandler that is an interface not fitting the callback-like function form. Using a *Func suffix would already have some implementations, be shorter, fit a bit closer to the functional nature of the call (this is subjective for sure), and wouldn't conflict with the existing instance of the ErrorHandler.

Additionally, our existing Bound* instruments used to be called Handlers*. This was changed so there wouldn't be conflict there, but there are still some lurking remnants (maybe just this so not the biggest deal 🤷) of this naming that might confuse if we went with *Handler.

@MrAlias MrAlias linked a pull request Aug 20, 2020 that will close this issue
@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 20, 2020

resolved in #1061

@MrAlias MrAlias closed this as completed Aug 20, 2020
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants