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 gender to name generator #551

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Conversation

MaicolBen
Copy link
Contributor

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?

@stympy
Copy link
Contributor

stympy commented Apr 15, 2016

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. :)

@MaicolBen
Copy link
Contributor Author

MaicolBen commented Apr 18, 2016

I had to check if parse doesn't return empty in the cases that first name is defined as single string (the locales that don't have gender), that's fine?

@MindRave
Copy link

@MaicolBen, @stympy hello! Is this coming anytime soon ? :) Thanks in advance!

@MaicolBen
Copy link
Contributor Author

MaicolBen commented Aug 16, 2017

Rebased, I made sure that the name's locals weren't updated

@MindRave
Copy link

@MaicolBen thank you for the speed! 👍 This looks great! I hope @stympy can merge this into the main repo soon. :)

@b264
Copy link
Contributor

b264 commented Aug 16, 2017

If it doesn't merge, there is male_name and female_name in https://github.com/factory-helper/factory-helper

@MindRave
Copy link

@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!

Copy link
Member

@vbrazo vbrazo left a 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?

@vbrazo
Copy link
Member

vbrazo commented May 16, 2018

@MaicolBen could you also update the CHANGELOG.md and add the title of this PR + your GitHub ID?

@MaicolBen
Copy link
Contributor Author

@vbrazo Done! You should add rubocop to travis or add Code Climate

end

def female_first_name
fetch('name.female_first_name')
Copy link
Member

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.

@faker-ruby faker-ruby deleted a comment from coveralls May 23, 2018
@faker-ruby faker-ruby deleted a comment from MaicolBen May 23, 2018
@vbrazo
Copy link
Member

vbrazo commented Jun 24, 2018

Any updates on this PR?

@MaicolBen MaicolBen force-pushed the add-gender-to-name branch 2 times, most recently from b2f9d2b to 7b29a97 Compare June 25, 2018 23:27
@MaicolBen
Copy link
Contributor Author

@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 (lib/locales/en/name.yml), that's a bad approach, I would take a random one from first_name

@vbrazo
Copy link
Member

vbrazo commented Jun 26, 2018

@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.

@vbrazo vbrazo merged commit 21c66ba into faker-ruby:master Jun 26, 2018
@vbrazo vbrazo self-requested a review July 19, 2018 01:21
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
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.

Add gender to name generator
5 participants