feat(person): split prefix into gendered versions#1665
feat(person): split prefix into gendered versions#1665ST-DDT merged 14 commits intofaker-js:nextfrom
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #1665 +/- ##
========================================
Coverage 99.63% 99.63%
========================================
Files 2262 2336 +74
Lines 240707 241081 +374
Branches 1095 1095
========================================
+ Hits 239825 240199 +374
Misses 861 861
Partials 21 21
|
ST-DDT
added
p: 1-normal
ST-DDT
left a comment
ST-DDT
left a comment
There was a problem hiding this comment.
Do you have a source for all of these, so I can verify them?
|
These PRs are quite huge, and some reviewers are tired of reviewing these large PRs. |
|
Is reviewing 30 small PRs any less tiresome than one big PR though? |
I think we somehow need to find a way where PRs are not getting larger than say max 50 files 🤔 Edit: I'm a heavy GitHub WebUI reviewer and do not use tools like VSCode |
|
We could "group" them by locale groups e.g. de + de_AT + de_CH + ... |
This could make sense as these would also attract more locale aware persons that e.g. in this example can target german locale reviews |
This PR still touches 29 different locales, even if you group. I feel for locale updates, it's not really possible to hand-review every change line-by-line. |
ST-DDT
changed the title
I think we should at least do a best effort job to review them (e.g. prevent curse words). Automated Review Scriptimport { faker } from "@faker-js/faker";
Array.prototype.equals = function (array) {
// if the other array is a falsy value, return
if (!array) return false;
// if the argument is the same array, we can be sure the contents are same as well
if (array === this) return true;
// compare lengths - can save a lot of time
if (this.length != array.length) return false;
for (var i = 0, l = this.length; i < l; i++) {
// Check if we have nested arrays
if (this[i] instanceof Array && array[i] instanceof Array) {
// recurse into the nested arrays
if (!this[i].equals(array[i])) return false;
} else if (this[i] != array[i]) {
// Warning - two different object instances will never be equal: {x:20} != {x:20}
return false;
}
}
return true;
};
// Hide method from for-in loops
Object.defineProperty(Array.prototype, "equals", { enumerable: false });
Object.entries(faker.locales).forEach(([locale, localeData]) => {
const {
prefix = [],
male_prefix = [],
female_prefix = [],
} = localeData.person ?? {};
console.log("------------------");
if (male_prefix) {
const gendered = Array.from(
new Set([...male_prefix, ...female_prefix])
).sort();
const ungendered = Array.from(new Set(prefix)).sort();
if (gendered.equals(ungendered)) {
console.log(locale, "Matches");
} else {
console.log(locale, "Does not match", gendered, ungendered);
}
}
});Could you please also "fix" |
|
I added a script at the top of the PR which spits out all current prefixes into a Markdown table to make it easier to review. |
|
Sources for the fact that certain locales do not use prefix titles: |
|
Thanks for the clarification. |
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
I left some comments, would like to have these questions addressed
after that => looking good to me 👍 the majority of this PR is a benefit
|
To confirm, do you want all prefix.ts files to be changed to use the new |
ST-DDT
left a comment
ST-DDT
left a comment
There was a problem hiding this comment.
Could you please switch
mergeArrays(male_prefix, female_prefix)to
mergeArrays(female_prefix, male_prefix)to use alphabetical order for the merge?
|
There seems to be merge conflcts. Can you please address them? |
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
I wont like to hold this back, so I will approve for now
We might need a bug issue for #1665 (comment) and handle it somehow
Maybe in a general way
Done: #893 (comment) |
|
Looks like a merge conflict crept in. Could you fix it please? |
xDivisionByZerox
left a comment
xDivisionByZerox
left a comment
There was a problem hiding this comment.
TBH, I reviewed for plausibility rather than checking every value on its own. For now, this looks good. If a person has more knowledge of a specific language, they can add/modify/revert in a later PR.
ST-DDT
changed the title
4 participants
Split from #1637
Because the default
enlocale does not haveperson/male_prefix.tsandperson/female_prefix.tsfiles, when usingfaker.person.fullName()you sometimes get a mismatch between the apparent sex of the first name and the title. For example:Mr. Cathy KleinorMrs. Nicholas LemkeThis PR adds male_prefix.ts and female_prefix.ts to all locales that currently have a prefix.ts (I used online sources for languages i was not familiar with to check if each prefix was male or female or generic like "Dr"`)
It also clarifies the documentation to make it clear that some locales dont have prefixes and so
faker.person.prefix()could return undefined.I wrote a small script to spit out a Markdown table to be able to more easily review the prefixes for all locales.
as of 71bf044