Skip to content

Remove functional option caller name match expectation - #1381

Closed
dillonstreator wants to merge 2 commits into
stretchr:masterfrom
dillonstreator:fix-1380
Closed

dillonstreator wants to merge 2 commits into
stretchr:masterfrom
dillonstreator:fix-1380

Conversation

@dillonstreator

@dillonstreator dillonstreator commented May 11, 2023

Copy link
Copy Markdown
Contributor

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

  • Removes expectation around functional options caller name matching as it is not guaranteed and not truly asserting the functional options function name.
  • Adds test to cover additional use case.
  • Introduces a strict length check on the function options.

Motivation

Fix bug and improve test to cover additional use cases.

Related issues

Closes #1380

@dillonstreator

Copy link
Copy Markdown
Contributor Author

@nbaztec curious to get your feedback on this as you implemented most of the initial version of functional options support

@nbaztec

nbaztec commented May 11, 2023

Copy link
Copy Markdown
Contributor

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.

@dillonstreator

Copy link
Copy Markdown
Contributor Author

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.

@dolmen

dolmen commented Jul 5, 2023

Copy link
Copy Markdown
Collaborator

@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 dolmen added Changes Requested pkg-mock Any issues related to Mock labels Jul 5, 2023
@dillonstreator

Copy link
Copy Markdown
Contributor Author

@dolmen thanks for the heads-up - rebased 👍

@dolmen

dolmen commented Jul 25, 2023

Copy link
Copy Markdown
Collaborator

Please don't merge master into your branch.

Instead:

git remote update
git rebase origin/master
git push -f

SuperQ and others added 2 commits August 12, 2023 23:46
Test old Go versions all the way back to 1.17.

Signed-off-by: SuperQ <superq@gmail.com>
@dillonstreator

Copy link
Copy Markdown
Contributor Author

Please don't merge master into your branch.

Instead:

git remote update
git rebase origin/master
git push -f

@dolmen executing this wiped my changes

@dolmen

dolmen commented Oct 16, 2023

Copy link
Copy Markdown
Collaborator

No more patch in this MR :(

@dolmen

dolmen commented Dec 10, 2024

Copy link
Copy Markdown
Collaborator

The original patch is still available here: 1ec5341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg-mock Any issues related to Mock

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Functional Options testing broken for indirect calls

5 participants