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

Add option to allow\disallow mocking methods unnecessarily for default expectations defined through quick definitions #1063

Conversation

danydev
Copy link
Contributor

@danydev danydev commented May 21, 2020

…t expectations set through quick definitions

@danydev danydev force-pushed the add-option-to-enforce-call-for-quick-definitions branch 5 times, most recently from 0ab1e3c to 2b6c311 Compare May 25, 2020 15:24
Copy link
Collaborator

@robertbasic robertbasic left a comment

Choose a reason for hiding this comment

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

Hey @danydev, thanks for this PR! My biggest issue with it are the really long constant names that reduce the readability of the code, so I left some feedback to try and make it a bit better. What do you think?

library/Mockery/Configuration.php Outdated Show resolved Hide resolved
library/Mockery/Configuration.php Outdated Show resolved Hide resolved
library/Mockery/Container.php Outdated Show resolved Hide resolved
library/Mockery/Configuration.php Outdated Show resolved Hide resolved
@davedevelopment
Copy link
Collaborator

I keep getting lost between the issue (#1056) and this PR...

Regarding the API and it's extensibility, this is a very narrow use case, so I wouldn't worry about it. Mockery has been going a long time and you're the first to request this, so I'm not overly concerned about making the feature extensible going forward.

The names do look messy, maybe introduce a small object to wrap the quick definition settings?

Mockery::configuration()
    ->quickDefinitions()
    ->shouldBeCalledAtLeastOnce(); // pass a boolean to set, no args to get

@danydev danydev force-pushed the add-option-to-enforce-call-for-quick-definitions branch from 2b6c311 to d621e43 Compare June 8, 2020 12:09
@danydev
Copy link
Contributor Author

danydev commented Jun 8, 2020

@davedevelopment just performed the changes!

@davedevelopment
Copy link
Collaborator

I'm not a fan of methods that change semantic based on values of their parameters (i.e. "I'm a getter unless you pass me a value and then I'm setter"). But that's fine for me if that's fine for you.

Generally I agree, but I tend to let it slide for what I would consider to be PHP's lack of (reasonable) support for the Uniform access principle.

Will give @robertbasic a day to object, but I'm happy to merge.

Copy link
Collaborator

@robertbasic robertbasic left a comment

Choose a reason for hiding this comment

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

This looks much better now, thanks @danydev!

@davedevelopment
Copy link
Collaborator

@danydev one last request, please could you add a quick note to the CHANGELOG.md file?

Thanks

@danydev danydev force-pushed the add-option-to-enforce-call-for-quick-definitions branch from d621e43 to fbb6697 Compare June 9, 2020 09:41
@danydev
Copy link
Contributor Author

danydev commented Jun 9, 2020

@davedevelopment done

@danydev danydev force-pushed the add-option-to-enforce-call-for-quick-definitions branch from fbb6697 to a9592ed Compare June 9, 2020 09:42
@davedevelopment davedevelopment merged commit bebae3d into mockery:master Jun 9, 2020
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.

None yet

3 participants