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(locale): add city names for th #2075

Merged
merged 5 commits into from
Apr 22, 2023
Merged

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Apr 21, 2023

Adds city names for th. This stops fakerTH.location.city() sometimes throwing when #2004 is merged, as th had no city patterns, so fell back to en patterns, but sometimes that would select a pattern with a city_prefix, which was empty in th.

@matthewmayer matthewmayer requested a review from a team as a code owner April 21, 2023 13:32
@matthewmayer matthewmayer changed the title locale(location): add city names for th feat(locale): add city names for th Apr 21, 2023
@matthewmayer matthewmayer added c: feature Request for new feature p: 1-normal Nothing urgent m: location Something is referring to the location module labels Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #2075 (ea3d1bb) into next (16d611f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2075   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files        2536     2538    +2     
  Lines      242236   242253   +17     
  Branches     1298     1302    +4     
=======================================
+ Hits       241293   241330   +37     
+ Misses        916      896   -20     
  Partials       27       27           
Impacted Files Coverage Δ
src/locales/th/location/city.ts 100.00% <100.00%> (ø)
src/locales/th/location/city_name.ts 100.00% <100.00%> (ø)
src/locales/th/location/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Apr 22, 2023

This needs updating.

@matthewmayer
Copy link
Contributor Author

matthewmayer commented Apr 22, 2023

Test failure seems unrelated see #2080

@ST-DDT
Copy link
Member

ST-DDT commented Apr 22, 2023

Test failure seems unrelated see #2080

Yeah, one of the rare cases where a float() is an int.

ST-DDT
ST-DDT previously approved these changes Apr 22, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the city prefixes/suffixes not applicable to th in general or did you remove them because we dont need them for the current pattern?

@ST-DDT ST-DDT requested review from Shinigami92 and a team April 22, 2023 08:10
@matthewmayer
Copy link
Contributor Author

Are the city prefixes/suffixes not applicable to th in general or did you remove them because we dont need them for the current pattern?

Both

@ST-DDT
Copy link
Member

ST-DDT commented Apr 22, 2023

If it is not applicable, then keeping the data as [] shows to the other developers that this is intentionally absent.
Also the [] does not really add big memory or source code overhead, so I think we should keep them.

@matthewmayer
Copy link
Contributor Author

My assumption was that was only needed for definitions which have public methods. Now that citysuffix and cityprefix are no longer methods, seems like they could just be omitted for locales that don't need them?

@xDivisionByZerox
Copy link
Member

[...] Now that citysuffix and cityprefix are no longer methods, seems like they could just be omitted for locales that don't need them?

Yes they are indeed no longer accessible via public methods, BUT if the city suffixes/prefixes simply don't exist in the th locale they should be left in. This allows us to track which data is actually missing and which are not applicable. This will become easier after #2078.

@matthewmayer
Copy link
Contributor Author

Sure, restored

Shinigami92
Shinigami92 previously approved these changes Apr 22, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) April 22, 2023 11:12
@ST-DDT ST-DDT merged commit ed19bef into faker-js:next Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: location Something is referring to the location module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants