Copy name related code from Malaysia Person class to Singapore Person class #507
Conversation
…erits its names methods
|
This does not feel right. Both belong to another language so please lets not mix in between. If you want proper names, update the pr to reflect it in the locale itself |
|
so ur fine with me copying all the name related methods and variables over? which is pretty much everything except the malaysian Id method or can I hold an instance of the Malaysia person class in the singapore person class and proxy calls to it? let me know which u prefer and I update the pr |
Copy it over since both are seperate languages. For Faker 2.x which is the next major all will consist of language packs so there cant be a mix between those |
|
Alright, I have copied them over. |
ziming
changed the title
|
Please update the PR according the contribution guidelines. I see no tests at all etc |
|
oh ok I will copy the tests over too later. am sick now |
Fine, but also please review the test to see if they are valid. We will review this as new code |
|
Hi I just went to the ms_MY Person test file and found that it has no test for it's name methods https://github.com/FakerPHP/Faker/blob/main/test/Faker/Provider/ms_MY/PersonTest.php I then went to check a few other related locales since Singapore and Malaysia is largely made up of Chinese, Malays/Muslim, Indians, English/Christian names. English: (no name tests) https://github.com/FakerPHP/Faker/blob/main/test/Faker/Provider/en_GB/PersonTest.php Chinese: (no name tests) Malay: (no name tests) Indian: (no name tests) So this mean i have to try write my own tests for the names, which will take longer, hope you don't mind waiting until my covid is over. So it may take longer than this week. |
|
No worries! Better to have solid code. Will pin the PR for the time being. Take care and recover well! |
|
Hi, added the tests. @pimjansen |
|
Hmm I should have used assertIsString instead of assertNotEmpty for some of them Should I change all to use assertIsString or just the ones that may return an empty string? |
|
Hi @pimjansen I changed the assertions for those where the result might be empty string to assertIsString |
|
Hi @pimjansen anything else I need to do? Thank you |
|
I did not look yet, currently i am on leave but i will review once im back |
|
hi @pimjansen hope u enjoyed ur leave. is this okay now? |
|
For some reason the pipeline is not running. Weird. |
|
Anything I can do to help resolve that or is that something on the faker team side? @bram-pkg |
|
You could try making an empty commit. |
|
As you can see I tried to make an empty commit but the github actions still did not run |
|
Seems like the pipeline jobs are queued, but are "awaiting approval from a maintainer"? Maybe connected to this: https://github.blog/changelog/2021-04-22-github-actions-maintainers-must-approve-first-time-contributor-workflow-runs/ ? |
|
Please fix the tests for PHP 7.4 and the other pipeline actions |
I approved whehe |
| @@ -10,6 +10,96 @@ | |||
| */ | |||
| final class PersonTest extends TestCase | |||
| { | |||
| public function testFirstNameMaleMalay() | |||
| { | |||
| self::assertNotEmpty(Person::firstNameMaleMalay()); | |||
There was a problem hiding this comment.
They will never be empty due to how interfacing works. Please assert actual values with seeding instead.
Currently Singapore Person class has no name related methods, which mean that when you call
$this->faker->name()
you will get either Jane Doe or John Doe, this also cause issues when you do $this->faker->unique()->name()
since you only have 2 names in the default Person super class.
Now, Malaysia and Singapore are 2 neighbour countries that share many similarities when it come to person names. Residents of both countries often travel to each other for leisure and work reasons too. Making it a great class for the Singapore Person class to inherit from.