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 support for defining factory parameters #428

Closed
wants to merge 12 commits into from

Conversation

@predakanga
Copy link
Contributor

predakanga commented Jul 9, 2016

Fixes #362, matches your method name conventions, etc wherever possible

@mnapoli
Copy link
Member

mnapoli commented Jul 9, 2016

Hi! That looks really good! I have a few comments, I'll put them in the diff directly.

@mnapoli mnapoli added the enhancement label Jul 9, 2016
*
* @return FactoryDefinitionHelper
*/
public function factoryParameter($parameter, $value)

This comment has been minimized.

Copy link
@mnapoli

mnapoli Jul 9, 2016

Member

I think factory(MyFactory::class)->parameter('foo', 'bar') would be simpler than factory(MyFactory::class)->factoryParameter('foo', 'bar').

This comment has been minimized.

Copy link
@predakanga

predakanga Jul 10, 2016

Author Contributor

Happy to change it - I initially wrote it as ->parameter, changed it to ->factoryParameter to match methodParameter and constructorParameter

@@ -201,7 +201,7 @@ public function test_not_enough_parameters()

/**
* @expectedException \Invoker\Exception\NotCallableException
* @expectedExceptionMessage "foo" is neither a callable nor a valid container entry
* @expectedExceptionMessageRegExp /('|")foo('|") is neither a callable nor a valid container entry/

This comment has been minimized.

Copy link
@mnapoli

mnapoli Jul 9, 2016

Member

Is there a reason those changes are necessary? I think it might be because the quotes have changed between the time you created the branch and when you created the pull request. If you rebase against master it should all be good.

This comment has been minimized.

Copy link
@predakanga

predakanga Jul 10, 2016

Author Contributor

I'm not sure if it's an OS issue, but even on master those changes are needed for me.
The actual issue is that var_export() (here) is returning 'foo' for me on both OS X and Linux, where the test assumes it should return "foo"

I've uploaded my test logs for Linux (docker) and OS X to demonstrate

This comment has been minimized.

Copy link
@mnapoli

mnapoli Jul 10, 2016

Member

Right I was running tests on master locally but it was passing, I had a deeper look and it's actually the latest version of PHP-DI/Invoker that changed the quotes -_-

I fixed it on master: f3acb12 If you rebase against master it should be good now. Sorry for that :)

@mnapoli mnapoli added this to the 5.4 milestone Jul 9, 2016
@mnapoli
Copy link
Member

mnapoli commented Jul 9, 2016

This is a really great pull request! Could you add a few functional test cases (here)?

@mnapoli
Copy link
Member

mnapoli commented Jul 9, 2016

Oops almost forgot: could you update the documentation here too: https://github.com/PHP-DI/PHP-DI/blob/master/doc/php-definitions.md#factories

@predakanga
Copy link
Contributor Author

predakanga commented Jul 10, 2016

Hey, thanks for the feedback!

I'll update the PR shortly, though I'm not sure if rebasing will need me to create a new one - I selected to merge into PHP-DI:5.3 instead of master.

@mnapoli
Copy link
Member

mnapoli commented Jul 10, 2016

Is there a reason you wanted to merge that into 5.3? To me it's a big feature, it deserves a minor version so it should go in master so that we can release it as 5.4. I'm afraid you'll have to close this one and reopen a new one though, I don't think you can edit the target branch?

I had a look at the latest commits and the docs + new tests are awesome!

@predakanga
Copy link
Contributor Author

predakanga commented Jul 10, 2016

There's no real reason to merge it into 5.3, I just made the initial fork from there.

I'm rebasing it now, I'll open a new PR with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.