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

#419: Create a service locator for clients to be used in the commands. #433

Merged
merged 6 commits into from
Jun 19, 2018

Conversation

Basster
Copy link
Contributor

@Basster Basster commented Jun 6, 2018

I hope master is fine as target branch.

@@ -1,6 +1,6 @@
parameters:
env(REDIS_URL): redis://localhost
env(REDIS_PROFILE): 2.6
env(REDIS_PROFILE): '2.6'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should wait on a fix from Symfony for this : symfony/symfony#27470
It's a nice thing to automatically know when BC break are introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it, since it wasn't directly related with the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add it again, as travis fails on PHP 7.1 and 7.2 when a float value is given!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can just keep it out, we're stalling new release until Symfony 4.1.1 is out anyway.

}

/**
* @test
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure this project is using this convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope it's not a big deal. I find the test method names generally much more readable without the test... prefix. Same reason why I prefer the prophecy mocks over phpunit mocking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry but preferring existing convention here, and all remaining tests use test prefixes.

remove unrelated .yaml change
made REDIS_PROFILE a string again, as travis fails on PHP 7.1 and 7.2

$clientLocatorDefinition = $this->createClientLocatorDefinition($container, $clients);

$this->passClientLocatorToSncRedisCommans($container, $clientLocatorDefinition);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in function name.

@Basster
Copy link
Contributor Author

Basster commented Jun 19, 2018

@curry684 I renamed the test methods and fixed the typo.

@curry684
Copy link
Collaborator

Thanks!

@curry684 curry684 merged commit 6db83c0 into snc:master Jun 19, 2018
@Basster Basster deleted the bugfix/#419-service-locator branch June 30, 2018 12:02
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