French: added translated color for french language #466
Conversation
|
Cant these classes all extend from FR_fr to remove duplication? |
|
Good point @pimjansen will do! |
|
@pimjansen Let me know if it's ok for you. I used |
Lets use FR_fr instead since its the base locale for it |
|
@pimjansen Done! |
|
PR looks ok besides the two mentioned topics |
|
@pimjansen Done! Please, note it was due to the CS linter ran with PHP 8.1.4, it sure is off topic but you may like to checks this 😄 |
It should be executed with the lowest supported php version in the scripts (see workflows). This due support for older versions and thus syntax that can change over time |
|
One last thing i did not mentioned earlier (sorry for that though its in the guide). Please ensure the new code is covered by unittests |
|
@pimjansen Do you want me to test an array? That's not even testable 🥲. First time I truly contribute to an open-source project, now I understand why most devs won't do this in their career. (sorry for that though, that's my feeling) Here, a simple test: 9cd51d8 |
|
Sorry that you feel this way! But as mentioned in the contribution guide which you manually checked it states:
Since this is mandatory in libraries used all over the place. The test now actually is testing is something is a string which actually does not test much. Faker works with seeding so please assert an actual value from the seed so we can ensure this does not change. You might think that you are just testing an array value but there could actually be quite some logic behind it. This test is both for the safeColor as allColor methods from the Faker Generator |
|
@pimjansen That's ok, it's more a question of "how" than "why/what". Can you provide me an example (or something similar) where I can see how to seed? Since it's color, I don't have many methods to parse all the array, I only have string with |
|
Seeding is done automatically in the base test class. Look at this test for example: https://github.com/FakerPHP/Faker/blob/main/test/Faker/Core/DateTimeTest.php |
|
So, @bram-pkg my tests are ok? |
|
Not quite, make sure to compare them against the output of your function. Since it's being seeded, it will always be the same. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions. |
|
Don't. I will find time to write those tests |
|
Honestly @bram-pkg and @pimjansen, I don't quite understand. I didn't even write some functions in this PR. I only translated some strings. Can you pin me exactly what did you expect? The |
We are changing the dataset so we want to ensure it stays intact and does what expected. You now added some regex to see if its a valid string which on its own is fine. Since Faker is using seeding it means that a testcase always returns the same result. Therefor i suggest to test an actual value of the generator. In your case assert that the generator returns the value which is returned while calling it with a specific seed. |
|
@RSickenberg can you allow us to make changes to your PR to finish it? |
@pimjansen Sure, please do. I don't have time to contribute yet 😔 |
You have to allow this since i cant at this moment |
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions. |
|
@pimjansen Any updates ? |
|
Thank you @pimjansen ❤️. |
Is it clear for you how the seeding works now and the diff i did in the test? |
|
@pimjansen Yup, not gonna lie, I thought the call was about to return random choices instead of the one asserted. |
No worries, now you know for the future 😋 |
What is the reason for this PR?
Author's checklist
Summary of changes
I simply added a list of colors we use in french.
Review checklist