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

feat(location): update en county list #2238

Merged
merged 5 commits into from Sep 1, 2023
Merged

Conversation

matthewmayer
Copy link
Contributor

re #2207 #1944

Updates the en county list to be a lomger list of US-esque county names instead of a
truncated list of GB county names.

@matthewmayer matthewmayer requested a review from a team as a code owner July 5, 2023 08:05
@matthewmayer matthewmayer changed the title locale(location): update en county list to match en_US feat(location): update en county list to match en_US Jul 5, 2023
@matthewmayer matthewmayer self-assigned this Jul 5, 2023
@matthewmayer matthewmayer added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: location Something is referring to the location module labels Jul 5, 2023
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #2238 (3a0fc17) into next (8e5bc22) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2238      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2760     2760              
  Lines      251164   251266     +102     
  Branches     1081     1083       +2     
==========================================
+ Hits       250176   250274      +98     
- Misses        961      965       +4     
  Partials       27       27              
Files Changed Coverage Δ
src/locales/en/location/county.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

import-brain
import-brain previously approved these changes Jul 8, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jul 9, 2023

Shouldn't we mix other states counties in as well otherwise en == en_US.
Maybe lets discuss this in the next team meeting.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jul 9, 2023
@matthewmayer
Copy link
Contributor Author

We don't do that for state()

@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2023

There seems to be merge conflicts. Could you please fix them?

@import-brain import-brain added the needs rebase There is a merge conflict label Aug 4, 2023
@import-brain import-brain removed the needs rebase There is a merge conflict label Aug 5, 2023
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Aug 17, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 17, 2023

Team decision

We will use both the "British" and US county names for en.

@matthewmayer
Copy link
Contributor Author

Done.

Note this is a good example of why you don't always want to sort definitions alphabetically re #2265

@ST-DDT ST-DDT changed the title feat(location): update en county list to match en_US feat(location): update en county list Aug 20, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) September 1, 2023 18:30
@ST-DDT ST-DDT merged commit 6bb4775 into faker-js:next Sep 1, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants