Skip to content
This repository has been archived by the owner on Jul 29, 2022. It is now read-only.

[RFR] Inject store arguments in Behat hooks non step methods #24

Merged
merged 10 commits into from
Apr 26, 2017
Merged

[RFR] Inject store arguments in Behat hooks non step methods #24

merged 10 commits into from
Apr 26, 2017

Conversation

vincentchalamon
Copy link
Collaborator

@vincentchalamon vincentchalamon commented Apr 4, 2017

Fix #23

@vincentchalamon vincentchalamon self-assigned this Apr 4, 2017
@vincentchalamon vincentchalamon requested a review from gorghoa April 4, 2017 08:27
Copy link
Owner

@gorghoa gorghoa left a comment

Choose a reason for hiding this comment

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

thanks @vincentchalamon, this is very nice work 👍

$environment;
$hook;
$arguments;
$errorReportingLevel;
Copy link
Owner

Choose a reason for hiding this comment

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

I don’t quite see the point of those expressions 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, it's still WIP ;)

@vincentchalamon vincentchalamon changed the title [WIP] Inject store arguments in Behat hooks non step methods [RFR] Inject store arguments in Behat hooks non step methods Apr 24, 2017
@vincentchalamon
Copy link
Collaborator Author

Unit tests in progress…

*/
public function dispatchScopeHooks(HookScope $scope)
{
$results = array();

Choose a reason for hiding this comment

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

Use short syntax!

continue;
}

$paramsKeys = array_map(function($element) {

Choose a reason for hiding this comment

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

What about foreach vs array_map()?

return $setup;
}

$hookCallResults = $this->dispatcher->dispatchScopeHooks(new BeforeScenarioScope($env, $feature, $scenario));

Choose a reason for hiding this comment

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

This var is useless.

return $teardown;
}

$hookCallResults = $this->dispatcher->dispatchScopeHooks(new AfterScenarioScope($env, $feature, $scenario, $result));

Choose a reason for hiding this comment

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

This var is useless.

@vincentchalamon
Copy link
Collaborator Author

vincentchalamon commented Apr 25, 2017

Scrutinizer patches are wrong: http://www.php-fig.org/psr/psr-2/#closures
Some Scrutinizer issues are wrong (duplicate code, PHPUnit mocks), but the other issues should be fixed in another PR (https://goo.gl/cA4uEC)

Copy link
Owner

@gorghoa gorghoa left a comment

Choose a reason for hiding this comment

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

Nice :) 👍

@gorghoa
Copy link
Owner

gorghoa commented Apr 26, 2017

@meyerbaptiste, ok for you?

@gorghoa
Copy link
Owner

gorghoa commented Apr 26, 2017

@Gregcop1, If you have some time to spend, could you verify that the PR fixes your use case? (If not, just trust @vincentchalamon’s work and everything will be fine I’m sure)

@gorghoa
Copy link
Owner

gorghoa commented Apr 26, 2017

Note. I’m okay with the small BC breaks due to namespace reorganization (the Exception chiefly).

I suggest to ship it to minor version anymay, even if it conflicts with semver. I expect no one will feel the pain since it’s deeply internal subject and the user base is still small ;)

@gorghoa gorghoa merged commit d8bfad9 into gorghoa:master Apr 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants