Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Test could run with every provider #311

Closed
wants to merge 1 commit into from
Closed

Test could run with every provider #311

wants to merge 1 commit into from

Conversation

fonsecas72
Copy link
Contributor

Faker has (many) providers which email is returning invalid emails.
But, as Faker does not have tests for every provider this is not noticed.
Well done or not, I've created a test that will run every existent provider. Off course this test will fail right at the beginning.
This could also be useful to test a unique "toAsscii" method that would be used in every provider.
Anyway, this is just me playing around (I enjoy it anyway) and maybe you have a better idea or already have though of this.
Feel free to close this if useless and sorry for wasting you time.

@fzaninotto
Copy link
Owner

Great initiative. But you'll have to go further:

  • you should do the same for username
  • existing localized unit tests for email or username should be removed
  • and obviously, when translitteration is missing, it should be added to pass this PR to green...

@fonsecas72
Copy link
Contributor Author

Sill not good to go but, for now only username is "really" fixed.
What I did was to create a global "toAscii" method (I called it "convertToNormal") that should convert almost every char to their normalized form.
This method should now be called from the provider that need it. I did this so maintain behaviour for providers that implemented their own "toAscii" and to not enforce the conversion of chars when not needed.
But I was not able to convert all chars and mark those tests as incomplete. They follow:

 'bg_BG',   'hy_AM',   'sr_RS',  'zh_CN',  'bn_BD',    'sr_Cyrl_RS'

I will do the same for email. Currently email is enforcing conversion but I just leave it like that only to be sure that I don't loose my work and because I wanted to ask you if you think this is ok first.
What do you think? Have a better solution?

@fonsecas72
Copy link
Contributor Author

ok, I'm close to done.
I'll squash this for you to code-review as soon as I can.

@fonsecas72
Copy link
Contributor Author

This is ready for you now.
Resume:
Created a method "convertToNormal" in Faker\Provider\Internet that should be able top convert chars for most languages.
I call it convertToNormal just to avoid php errors with toAscii method. My idea is to have a global toAscii method and then overwrite it when needed in each locale.
We can also copy this method inside every locale but I think that DRY...
I had to fix some locales but I could not fix all of them:

        'bg_BG',     'bn_BD',     'sr_Cyrl_RS',     'sr_RS',     'zh_CN',

This locales will be skipped from test.
Tell me what do you think. Thanks.

@fonsecas72 fonsecas72 mentioned this pull request May 14, 2014
@fzaninotto
Copy link
Owner

  • You can't do half the translitteration in the base provider, and the other half in localized providers. It doesn't make sense. Once you start refactoring translitteration to the baseProvider, you must remove all toAscii() implementations in localized providers.
  • You must detect if intl is installed, and use transliterator_transliterate() then - it'll be faster than string replacement.
  • The base username and email formatters, should call the translitteration method. That way, you can remove all the extra work done in the localized formatters, and you PR will turn from green to red, which is good!
  • Your PR needs a rebase.

@fonsecas72
Copy link
Contributor Author

Just so see if I understood:
So username and email will always call transliterator_transliterate ?

I'll look into this again very soon.

@fzaninotto
Copy link
Owner

yep, that's the idea.

@fonsecas72
Copy link
Contributor Author

Moved to #333

@fonsecas72 fonsecas72 closed this May 15, 2014
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.

2 participants