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.

Sign up for GitHub

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

Jump to bottom

Add gender #443

Closed
CodeWritingCow opened this issue Nov 2, 2017 · 18 comments
Closed

Add gender #443

CodeWritingCow opened this issue Nov 2, 2017 · 18 comments
Assignees
@CodeWritingCow
Labels
enhancement ideas

Comments

@CodeWritingCow
Copy link
Collaborator

Hi @bluzi and everyone: Should we add "sex" or "gender" to the JSON file structure?

Users might want to know whether a name is male, female or unisex. Especially if the name is from a language foreign to them.

Just a suggestion ...
@bluzi
Copy link
Owner

bluzi commented Nov 2, 2017

Hey @CodeWritingCow,

I agree.
In your opinion, what would be the best way to implement it? We need to consider contributor's convenience and readability.

I mean, it could be something like: male female unisex, but it may cause trouble because or typos or people who write females or something like that.
Alternatively, we could use numbers, such as: 0 = unisex, 1 = male, 2 = female. But that damages readability so I tend to go with the first option.

Any ideas?

@roastchicken
Copy link
Collaborator

Single characters, such as m, f, and u, could reduce the incidence of misspellings while still keeping a decent amount of readability.

@CodeWritingCow
Copy link
Collaborator Author

CodeWritingCow commented Nov 3, 2017 •
edited
Loading

@roastchicken I was thinking the same thing. Single characters are a good compromise between using numbers and spelling entire words.

If we do use m, f and u, we should explain their meaning in readme.md.

@bluzi
Copy link
Owner

bluzi commented Nov 3, 2017

Sounds good to me, I'm in!

Anyone wants to implement it and create a PR?
You should:

  • Update the documentation
  • Add a test
  • Create an issue for adding genders

@CodeWritingCow
Copy link
Collaborator Author

@bluzi I'll do it tonight!

@bluzi
Copy link
Owner

bluzi commented Nov 3, 2017

Awesome!

@CodeWritingCow
Copy link
Collaborator Author

Hi @bluzi, I implemented sex in my fork's readme.md and test.js. However, I'm having trouble making a PR.

This might be due to a previous PR that's open at #225 ... Can you close that PR for me? My apologies.

@bluzi
Copy link
Owner

bluzi commented Nov 4, 2017

Hey @CodeWritingCow,

It looks like your PRs go mixed up, because you pushed everything to the master branch of your fork.
So all of your changes are at #225

Anyway, that's fine, the problem is I cannot merge the change because the tests failing. That's probably because you made the sex field mandatory. I think it should be optional, at least until we have it in the whole collection.

CodeWritingCow added a commit to CodeWritingCow/name-db that referenced this issue Nov 4, 2017
@CodeWritingCow
Make sex field optional for now
c21bac2 
Comment out tests for sex field. They are failing because names in
collection don't have that field. Issue bluzi#443
@CodeWritingCow
Copy link
Collaborator Author

Hi @bluzi,

I've commented out the sex field tests for now. All the other tests have passed.

@CodeWritingCow
Copy link
Collaborator Author

Opened issue #456 for adding genders.

@bluzi
Copy link
Owner

bluzi commented Nov 6, 2017

Hi @CodeWritingCow,

I'm thinking: maybe the field itself should be optional, but if the field exists, it should only contain f, m or u, so that's something you can add a test for.

After that you can merge it.

@bluzi bluzi mentioned this issue Nov 6, 2017
Merged
CodeWritingCow added a commit to CodeWritingCow/name-db that referenced this issue Nov 7, 2017
@CodeWritingCow
Add tests to check validity of gender
fcdacae 
Tests run only on name files that contain a gender value. Issue bluzi#443
Update README.md to describe sex field as optional.
@CodeWritingCow
Copy link
Collaborator Author

CodeWritingCow commented Nov 7, 2017 •
edited
Loading

Hi @bluzi,

I followed your suggestion and made the field optional. If the field exists in a name field, then the test checks it for validity.

If the field contains only "", should it be allowed to pass the test? Some contributors might still include "sex": "" if they aren't sure of a name's gender.

@bluzi
Copy link
Owner

bluzi commented Nov 7, 2017 •
edited
Loading

If you ask me, I think it should be illegal. If you don't know the sex, don't specify the sex property at all.

@CodeWritingCow

@roastchicken
Copy link
Collaborator

I agree with @bluzi, the sex field should require a value.

CodeWritingCow added a commit to CodeWritingCow/name-db that referenced this issue Nov 8, 2017
@CodeWritingCow
Update gender tests - Issue bluzi#443
1ce0b8b 
Sex field now requires a value to pass test. Also:
- Add error messages to explain why a test failed.
- Remove test for checking whether a sex value is a string. It's
duplicating part of the "should have correct structure" test.
@CodeWritingCow
Copy link
Collaborator Author

@bluzi and @roastchicken, I updated the test so sex field is optional. But, if a file includes the sex field, then it cannot be empty.

@chrisf chrisf mentioned this issue Nov 8, 2017
Closed
bluzi pushed a commit that referenced this issue Nov 8, 2017
@CodeWritingCow @bluzi
Update "michael.json" (#225)
4d4fecd 
* Add name "Jesus"

* Fix ISO language code

Use "cmn" for Chinese.

* Lowercase name

* Update 'Michael'

Add aliases and translation.
Merge "mitch" with "michael" because mitch is an alias for Michael.
Delete "mitch.json."

* Add gender to name file specs

Add gender to file specs in readme.md. Add tests to validate each JSON's
gender value.

* Update michael.json

Resolve merge conflict by using main repo's version of JSON file.

* Make sex field optional for now

Comment out tests for sex field. They are failing because names in
collection don't have that field. Issue #443

* Add tests to check validity of gender

Tests run only on name files that contain a gender value. Issue #443
Update README.md to describe sex field as optional.

* Update gender tests - Issue #443

Sex field now requires a value to pass test. Also:
- Add error messages to explain why a test failed.
- Remove test for checking whether a sex value is a string. It's
duplicating part of the "should have correct structure" test.
@bluzi
Copy link
Owner

bluzi commented Nov 9, 2017

Merged it, thanks a lot! @CodeWritingCow

@CodeWritingCow
Copy link
Collaborator Author

@bluzi you are very welcome! Thanks for your patience.

bluzi pushed a commit that referenced this issue Nov 10, 2017
@CodeWritingCow @bluzi
Add sex to names (#467)
aca21bc 
* Add name "Jesus"

* Fix ISO language code

Use "cmn" for Chinese.

* Lowercase name

* Update 'Michael'

Add aliases and translation.
Merge "mitch" with "michael" because mitch is an alias for Michael.
Delete "mitch.json."

* Add gender to name file specs

Add gender to file specs in readme.md. Add tests to validate each JSON's
gender value.

* Update michael.json

Resolve merge conflict by using main repo's version of JSON file.

* Make sex field optional for now

Comment out tests for sex field. They are failing because names in
collection don't have that field. Issue #443

* Add tests to check validity of gender

Tests run only on name files that contain a gender value. Issue #443
Update README.md to describe sex field as optional.

* Update gender tests - Issue #443

Sex field now requires a value to pass test. Also:
- Add error messages to explain why a test failed.
- Remove test for checking whether a sex value is a string. It's
duplicating part of the "should have correct structure" test.

* Add sex to names - Issue #456

* Add sex to names - Issue #456
arminkhoshbin pushed a commit that referenced this issue Nov 11, 2017
@CodeWritingCow @arminkhoshbin
Add sex to names (#474)
ec34399 
* Add name "Jesus"

* Fix ISO language code

Use "cmn" for Chinese.

* Lowercase name

* Update 'Michael'

Add aliases and translation.
Merge "mitch" with "michael" because mitch is an alias for Michael.
Delete "mitch.json."

* Add gender to name file specs

Add gender to file specs in readme.md. Add tests to validate each JSON's
gender value.

* Update michael.json

Resolve merge conflict by using main repo's version of JSON file.

* Make sex field optional for now

Comment out tests for sex field. They are failing because names in
collection don't have that field. Issue #443

* Add tests to check validity of gender

Tests run only on name files that contain a gender value. Issue #443
Update README.md to describe sex field as optional.

* Update gender tests - Issue #443

Sex field now requires a value to pass test. Also:
- Add error messages to explain why a test failed.
- Remove test for checking whether a sex value is a string. It's
duplicating part of the "should have correct structure" test.

* Add sex to 31 names

Issue #456

* Add sex to 24 names

Issue #456
bluzi pushed a commit that referenced this issue Dec 2, 2017
@CodeWritingCow @bluzi
Add names (#482)
f65e63f 
* Add name "Jesus"

* Fix ISO language code

Use "cmn" for Chinese.

* Lowercase name

* Update 'Michael'

Add aliases and translation.
Merge "mitch" with "michael" because mitch is an alias for Michael.
Delete "mitch.json."

* Add gender to name file specs

Add gender to file specs in readme.md. Add tests to validate each JSON's
gender value.

* Update michael.json

Resolve merge conflict by using main repo's version of JSON file.

* Make sex field optional for now

Comment out tests for sex field. They are failing because names in
collection don't have that field. Issue #443

* Add tests to check validity of gender

Tests run only on name files that contain a gender value. Issue #443
Update README.md to describe sex field as optional.

* Update gender tests - Issue #443

Sex field now requires a value to pass test. Also:
- Add error messages to explain why a test failed.
- Remove test for checking whether a sex value is a string. It's
duplicating part of the "should have correct structure" test.

* Add two male names

* Remove extra comma from JSON file
@roastchicken
Copy link
Collaborator

This was merged two years ago in #467. The architecture changes specifically were in the two commits Add gender to name file specs and Add tests to check validity of gender.

The blames and logs are a bit confusing because there are two identically named pull requests with very similar commits, #467 and #474. But I believe the second, #474, was only to add sex fields to more names and didn't actually implement the architectural changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@CodeWritingCow CodeWritingCow

Labels
enhancement ideas
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@roastchicken @CodeWritingCow @bluzi