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 providers for ms_MY #1976

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add providers for ms_MY #1976

wants to merge 1 commit into from

Conversation

ngbk1993
Copy link

What does this change

Add providers for ms_MY for the address,
person, phone_number and ssn.

This is porting from the faker php
https://github.com/fzaninotto/Faker

Tested with git diff --check and the code is formatted with black --line-length 120

What was wrong

The ms_MY provider is missing.

How this fixes it

Add the ms_MY provider for address, person, phone_number and ssn

Add providers for ms_MY for the address,
person, phone_number and ssn.

This is porting from the faker php
https://github.com/fzaninotto/Faker

Signed-off-by: Ng Boon Khai <ngbk1993@yahoo.com>
Copy link
Collaborator

@fcurella fcurella left a comment

Choose a reason for hiding this comment

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

Can you add sources for the data, and tests?

"{{firstNameFemaleIndian}} {{lastNameIndian}}",
)

firstNameMaleMalay_tuple = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add sources for this data?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @fcurella, they are ported from the php faker https://github.com/fzaninotto/Faker/blob/master/src/Faker/Provider/ms_MY/Person.php

can i just include the github link where it is ported it from?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can follow the instructions here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that would work 👍🏼

Copy link
Author

Choose a reason for hiding this comment

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

sure, let me add the faker php link as the source. will update at the next version with the tests file.

@ngbk1993
Copy link
Author

Can you add sources for the dat, and tests?

I can add some tests to it, but how do i run the test locally? can share the step?

@dancergraham
Copy link
Contributor

Can you add sources for the dat, and tests?

I can add some tests to it, but how do i run the test locally? can share the step?

I had to python -m pip install pytest freezegun validators and then python -m pip install . after each change before running pytest

@stefan6419846
Copy link
Contributor

In theory it should be easier as documented in the docs at https://github.com/joke2k/faker/blob/master/CONTRIBUTING.rst: Just install tox via pip and then just use the tox command which should take care of the remaining stuff.

@ngbk1993
Copy link
Author

ngbk1993 commented Jan 24, 2024

In theory it should be easier as documented in the docs at https://github.com/joke2k/faker/blob/master/CONTRIBUTING.rst: Just install tox via pip and then just use the tox command which should take care of the remaining stuff.

yes, the pip install step was somehow missing from the CONTRIBUTING.rst, we cant assume that everyone has the tox module installed.
image

Was trying to dry run the tox but somehow i'm getting this output not sure if it is expected
image

PS: The test is running now
image

@ngbk1993
Copy link
Author

Can you add sources for the dat, and tests?

I can add some tests to it, but how do i run the test locally? can share the step?

I had to python -m pip install pytest freezegun validators and then python -m pip install . after each change before running pytest

Was following stefan suggestion to try tox, would give this a try if the tox failed

@stefan6419846
Copy link
Contributor

Was trying to dry run the tox but somehow i'm getting this output not sure if it is expected

Yes, this is expected. What tox does here basically is to iterate over the configured Python versions and if it finds the corresponding interpreter, will create an isolated environment, install the required packages there and run the necessary steps like tests there.

yes, the pip install step was somehow missing from the CONTRIBUTING.rst, we cant assume that everyone has the tox module installed.

Feel free to submit a corresponding PR to improve the docs if you miss something there or something tends to be unclear and could be improved.

@github-actions github-actions bot added the stale label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants