refactor(locale): updates county patterns for en, en_US#1988
refactor(locale): updates county patterns for en, en_US#1988matthewmayer wants to merge 2 commits into
Conversation
matthewmayer
added
m: location
|
I'm not sure whether we should start faking county names. |
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
Looks like you need to run vitest snapshots
| 'Buckinghamshire', | ||
| 'Cambridgeshire', | ||
| ]; | ||
| export default ['{{person.last_name}} County']; |
There was a problem hiding this comment.
Normally I would expect to have County names with famous/memorial names
This now results just in a random name with not so much value as manually calling faker.person.lastName() + ' County' myself.
Theoretically this could also result in even stuff like Виноградов County when mixing ru on top of en 👀
So maybe in this case (for county) we maybe should just remove county from en and lead users to use the specific en_GB/en_US variants.
There was a problem hiding this comment.
I see your point, though
Виноградов Countyseems better thanCambridgeshire, which is what you would get at the moment forru. Eventually all locales could have 1st and 2nd level administrative areas added.- Faking counties potentially avoids the problem where people say "but there's not a Marion County, California, that's in Mississippi" ie the 1st and 2nd level admin areas don't match up.
- If we remove the county definitions from
encompletely won't you get 'a', 'b', 'c' in the default locale due to the issue with arrayElement?
There was a problem hiding this comment.
If we remove the county definitions from en completely won't you get 'a', 'b', 'c' in the default locale due to the issue with arrayElement?
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe last_name+" County" is not the best approach, but its similar to how faker.location.city() and faker.location.street() works in en. Second level admin areas in most countries are numerous enough that people would not be expected to know all their names (whereas generating completely fake states might seem weirder).
There was a problem hiding this comment.
IMO there are very few counties (and very few usecases for them).
If you ask for one, thrn you probably need one explicitly.
For city and street we have a method that generates them and one that returns existing ones.
There was a problem hiding this comment.
There are 3,006 counties in the US.
There was a problem hiding this comment.
If we are going for generated county names, we should have a separate method for that.
(Similar to city and street names)
The question is, do we need that?
There was a problem hiding this comment.
No (I find the city/cityNane and street/streetName confusing enough)
There was a problem hiding this comment.
Maybe just make the county definitions for en []
Then you get undefined for now which is less confusing than "b" or "Cambridgeshire" and we can note in the docs that this only works in select locales.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #1988 +/- ##
========================================
Coverage 99.62% 99.62%
========================================
Files 2450 2459 +9
Lines 239748 239936 +188
Branches 1272 1280 +8
========================================
+ Hits 238853 239041 +188
+ Misses 873 872 -1
- Partials 22 23 +1
|
|
Also we should probably add the US county names in a separate PR, than the rewrite/refactor. |
matthewmayer
added
the
do NOT merge yet
3 participants
part of #1944
Currently the
enfaker.location.county data is a truncated list of 6 English counties.Improve this by