-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 gender to name generator #551
Conversation
You don't need to modify all the locale files... as long as the old first_name method works across all locales, I'm happy. :) |
cba4339
to
0e7fada
Compare
I had to check if |
@MaicolBen, @stympy hello! Is this coming anytime soon ? :) Thanks in advance! |
0e7fada
to
5216bb1
Compare
Rebased, I made sure that the name's locals weren't updated |
@MaicolBen thank you for the speed! 👍 This looks great! I hope @stympy can merge this into the main repo soon. :) |
If it doesn't merge, there is |
@b264 thank you. Will check it out, at least until the PR is merged into faker itself. That, or I'll end up separating the male/female names into arrays and doing a ".sample" on them. 😅 In any case, thanks a lot! |
5216bb1
to
1d5324a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing 🥇 Nice work!
Could you rebase this branch and run Rubocop
to make sure that we're following a few good practices?
@MaicolBen could you also update the CHANGELOG.md and add the title of this PR + your GitHub ID? |
1d5324a
to
55ca35b
Compare
@vbrazo Done! You should add rubocop to travis or add Code Climate |
55ca35b
to
321cb35
Compare
a359def
to
a5d7731
Compare
end | ||
|
||
def female_first_name | ||
fetch('name.female_first_name') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaicolBen You should write tests for male_first_name
and female_first_name
. I know that they're pretty simple, but we need to have them in place and green to make sure that things are ok.
Any updates on this PR? |
b2f9d2b
to
7b29a97
Compare
7b29a97
to
bd2507e
Compare
@vbrazo Sorry, I was on vacation. Also, I rebased tons of time this PR since I created it (look at the creation date). While rebasing I saw the middle name was added copying from first names from both genders ( |
@MaicolBen we worked on the middle name locales and I remember that we copied this information from the web. Feel free to open a new PR and propose the change. |
This fixes #289
Not merge yet! First a question:
@stympy Does I have to distinguish the gender in all language files? Or put in them a alias to
first_name
instead?