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

Merge faker.location.city() and faker.location.cityName() #2021

Closed
ST-DDT opened this issue Apr 4, 2023 · 11 comments · Fixed by #2094, #2070, #2104 or #2118
Closed

Merge faker.location.city() and faker.location.cityName() #2021

ST-DDT opened this issue Apr 4, 2023 · 11 comments · Fixed by #2094, #2070, #2104 or #2118
Assignees
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Apr 4, 2023

Clear and concise description of the problem

From just the method names, it is confusing what each method does.

It also causes issues when a locale does not have a city_name_pattern or city_name locale file.

Suggested solution

Merge both methods into one. (Not sure about the name yet)

faker.location.cityName({ allowGenerated: boolean = true });

Alternative

  • Rename the methods to account for them generating real and generated city names respectively.

Additional context

Part of #1346

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: location Something is referring to the location module deprecation A deprecation was made in the PR labels Apr 4, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Apr 4, 2023
@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 4, 2023

There seem to 4 5 different ways this is done:

  1. Has a list of real cities in city_name, and the city pattern just points to that, so city() and cityName() do the same:
[
    'ar',          'az',
    'cz',          'de_AT',
    'de_CH',       'en_GH',
    'en_IN',       'fa',
    'fr',          'fr_CH',
    'fr_LU',       'he',
    'hr',          'id_ID',
    'lv',          'mk',
    'pl',          'pt_PT',
    'ru',          'sk',
    'sr_RS_latin', 'vi'
  ]
  1. Only city() patterns (cityName() will fall back to en)

2a) generated patterns only

[
'af_ZA', 'el',
    'en_AU', 
    'en_GB', 'en_IE',
    'en_US', 'fr_BE',
    'fr_CA', 'hy',
    'ja',    'nl',
    'nl_BE', 'pt_BR',
    'sv',    'zh_CN',
    'zh_TW', 'zu_ZA'
]

2b) real cities stored in city_prefix

[
'en_AU_ocker',    'en_NG',     'en_ZA',    'es',    'es_MX',
]

3). Has both fictional city patterns and a list of real city names

[
'de',    'dv',    'en',
    'en_CA', 'it',    'ka_GE',
    'ko',    'nb_NO', 'ne',
    'ro',    'tr',    'uk',
    'ur'
]
  1. Has no cities. city() often looks weird as it mixes a local name with an en suffix like Lukácscester
 [ 'en_BORK', 'fi', 'hu', 'th' ]

@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 4, 2023

i'd probably do something like
faker.location.city({ preferGenerated: boolean = false });

Then

  1. Group 1 locales always return the real cities
  2. Group 2 locales always return the generated cities
  3. Group 3 locales return the real or generated cities depending on the value of the parameter
  4. Group 4 locales return the real or generated cities from fallback language depending on the value of the parameter

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 4, 2023

I also considered not having an option and only using the pattern file to determine the result.

@matthewmayer
Copy link
Contributor

that's possible but then you'd need to decide what to do with existing type 3 locales which have both.

@matthewmayer
Copy link
Contributor

Split the group 2 into two groups, 2a and 2b. 2b is probably a bug, and these should be converted into type 1 (see #1471) before this is implemented.

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 6, 2023

Team Suggestion

  • We will merge both methods into one. Most likely city()
  • We will start without a generated/fictional parameter and will add that back when users request them. (This avoids breaking the API when removing the parameter if unneeded)
  • We will adjust the locale data name to explicitly refer to city_pattern instead of just city.

@matthewmayer
Copy link
Contributor

For group 3 locales that currently have both which one will you pick?

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 6, 2023

For group 3 locales that currently have both which one will you pick?

Real city names are just one option from the "generated" pattern list.

@matthewmayer
Copy link
Contributor

Ok so it would be a mix of real and generated?

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 13, 2023

Team Decision

We want this for v8.0.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Apr 13, 2023
@xDivisionByZerox xDivisionByZerox self-assigned this Apr 19, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 26, 2023

Two more things needs to be done for this:

  • Add existing city names to city patterns
  • Adjust the jsdocs to mention that this returns a city name (fictional or existing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
4 participants