-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Remove functional option caller name match expectation #1381
Conversation
@nbaztec curious to get your feedback on this as you implemented most of the initial version of functional options support |
I might've been too strict in ensuring that exactly the intended option is passed, ergo the explicit nature of providing the option name as well (which we removed as part of the original PR). As of now I cannot seem to think of a use-case where that might be absolutely necessary, as such if the other tests pass, perhaps we can go forward with it. |
Hi @boyan-soubachov, could you please review this PR and provide feedback? It's currently blocking one of the primary use cases for testing functional options. Your insights would be greatly appreciated. |
@dillonstreator Please rebase this PR as it includes gofmt 1.20 formatting changes that have already been applied. Those change pollute the diff and make reviewing harder. |
@dolmen thanks for the heads-up - rebased 👍 |
Please don't merge master into your branch. Instead:
|
Test old Go versions all the way back to 1.17. Signed-off-by: SuperQ <[email protected]>
1ec5341
to
c410dd2
Compare
@dolmen executing this wiped my changes |
No more patch in this MR :( |
Summary
Fixes #1380 by removing expectations around optional functions calling function names. These assertions are not necessary to satisfy the testing use case for functional options and are in fact just outright incorrect due to the curried nature of the optional function. The time at which the return function from the optional function is evaluated, the optional function name is no longer accessible/in-scope.
Changes
Motivation
Fix bug and improve test to cover additional use cases.
Related issues
Closes #1380