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-faker-integration: removed 'name' because it is an existing faker option #22

Merged
merged 2 commits into from
Mar 17, 2017

Conversation

franziskahahn
Copy link
Contributor

Hey guys,
I saw that "name" is already (is it new, I never saw it before?! :D) an "official" faker formatter.

So I deleted the self-written formatter.
I decided to keep the configuration skeleton because I am sure we will need individual stuff in the further development of slimdump.

@coveralls
Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage decreased (-0.7%) to 68.707% when pulling 3d6f8d6 on franziskahahn:feature/add-faker-integration into 8ef3fe0 on webfactory:master.

@franziskahahn
Copy link
Contributor Author

I think the decreased coverage check is not correct..

* configuration for method which are not part of faker itself
* configuration for method which are not part of faker itself.
* You can implement a new method "methodName" which returns for example a
* format: 'suffix' => 'methodName',
* @var array
*/
private $replacementOptions = [
Copy link
Member

Choose a reason for hiding this comment

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

Can't we remove this as well?

@mpdude
Copy link
Member

mpdude commented Mar 10, 2017

Sorry @franziskahahn, I somehow lost sight of this.

I only quickly skimmed FakerReplacer on my mobile phone, so maybe I am wrong. But, if we remove this special case handling, can't we get rid of even more code there? I mean, we need not keep hooks and extension points around for things we don't actually use (yet).

As we haven't had a release since your initial contribution, I see no Problem in removing stuff.

@franziskahahn
Copy link
Contributor Author

Hey @mpdude
I also lost sight of this. I didn't deleted it yet because I thought I can find a new option but now I think that faker already provides all necessary options. I will delete the stuff.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 68.421% when pulling b024103 on franziskahahn:feature/add-faker-integration into 8ef3fe0 on webfactory:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 68.421% when pulling b024103 on franziskahahn:feature/add-faker-integration into 8ef3fe0 on webfactory:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 68.421% when pulling b024103 on franziskahahn:feature/add-faker-integration into 8ef3fe0 on webfactory:master.

@franziskahahn
Copy link
Contributor Author

I removed the config array

@mpdude mpdude merged commit 9ccfdb9 into webfactory:master Mar 17, 2017
@mpdude
Copy link
Member

mpdude commented Mar 17, 2017

Danke @franziskahahn!

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.

3 participants