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(address): use localized fake pattern in city #948

Merged
merged 3 commits into from
May 16, 2022

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented May 12, 2022

This PR changes the address.city method to use the address.city locale data with fake to generate city names.

For that I checked, that all city files are named correctly (city -> patterns; city_name -> actual city names).
I did not check whether the specified patterns make sense or the city names are real.

I generated the missing city files with the original pattern from the method to ensure it does not break between versions.
I did not add '{{address.city_name}}' to the city patterns if both the city and city_name files exist (except for the en locale because it affected a test, see my second commit).
I did not remove it from the patterns if the city_name file is missing in that locale.

Everything in the first commit was generated by using this code:

generateLocales.ts
const processed = new Set<string>();

/**
 * Use this hook method to selectively update locale data files (not for index.ts files).
 * This method is intended to be temporarily overwritten for one-time updates.
 *
 * @param filePath The full file path to the file.
 * @param locale The locale for that file.
 * @param localePath The locale path parts (after the locale).
 */
function updateLocaleFileHook(
  filePath: string,
  locale: string,
  localePath: string[]
): void {
  if (processed.has(locale)) {
    return;
  }
  if (localePath[0] !== 'address') {
    return;
  }
  processed.add(locale);

  let cityPattern: string[];
  let cityNames: string[];

  const cityFiles = ['city', 'city_name', 'city_root'];
  for (const name of cityFiles) {
    if (existsSync(`src/locales/${locale}/address/${name}.ts`)) {
      let content: string[] =
        require(`../src/locales/${locale}/address/${name}`).default;
      unlinkSync(resolve(`src/locales/${locale}/address/${name}.ts`));
      if (content[0].includes('{{')) {
        for (const replaced of cityFiles) {
          content = content.map((e) =>
            e.replace(
              new RegExp(`{{address.${replaced}}}`, 'g'),
              '{{address.city_name}}'
            )
          );
        }

        cityPattern = content;
      } else {
        cityNames = content;
      }
    }
  }

  if (cityPattern == null) {
    if (cityNames == null) {
      cityPattern = [
        '{{address.cityPrefix}} {{name.firstName}}{{address.citySuffix}}',
        '{{address.cityPrefix}} {{name.firstName}}',
        '{{name.firstName}}{{address.citySuffix}}',
        '{{name.lastName}}{{address.citySuffix}}',
      ];
    } else {
      cityPattern = [
        '{{address.cityPrefix}} {{name.firstName}}{{address.citySuffix}}',
        '{{address.cityPrefix}} {{name.firstName}}',
        '{{name.firstName}}{{address.citySuffix}}',
        '{{name.lastName}}{{address.citySuffix}}',
        '{{address.city_name}}',
      ];
    }
  }

  let cityPatternContent = `export default ${JSON.stringify(
    cityPattern,
    undefined,
    '  '
  )};\n`;
  cityPatternContent = formatTypescript(cityPatternContent);
  writeFileSync(
    resolve(`src/locales/${locale}/address/city.ts`),
    cityPatternContent
  );

  if (cityNames != null) {
    let cityNamesContent = `export default ${JSON.stringify(
      cityNames,
      undefined,
      '  '
    )};\n`;
    cityNamesContent = formatTypescript(cityNamesContent);

    writeFileSync(
      resolve(`src/locales/${locale}/address/city_name.ts`),
      cityNamesContent
    );
  }
}

And executed it using this helper:

untilSuccess() {
        until "$@"; do :; done
}

untilSuccess pnpm run generate:locales
pnpm run generate:locales

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent c: locale Permutes locale definitions labels May 12, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone May 12, 2022
@ST-DDT ST-DDT requested review from a team May 12, 2022 17:04
@ST-DDT ST-DDT self-assigned this May 12, 2022
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #948 (c73cde6) into main (80d9a09) will increase coverage by 0.00%.
The diff coverage is 98.85%.

@@           Coverage Diff           @@
##             main     #948   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files        1957     1970   +13     
  Lines      209831   209927   +96     
  Branches      878      875    -3     
=======================================
+ Hits       209112   209211   +99     
+ Misses        699      697    -2     
+ Partials       20       19    -1     
Impacted Files Coverage Δ
src/locales/nb_NO/address/city_name.ts 100.00% <ø> (ø)
src/locales/vi/address/city_name.ts 100.00% <ø> (ø)
src/modules/address/index.ts 98.31% <50.00%> (-1.48%) ⬇️
src/locales/af_ZA/address/city.ts 100.00% <100.00%> (ø)
src/locales/af_ZA/address/index.ts 100.00% <100.00%> (ø)
src/locales/el/address/city.ts 100.00% <100.00%> (ø)
src/locales/el/address/index.ts 100.00% <100.00%> (ø)
src/locales/en/address/city.ts 100.00% <100.00%> (ø)
src/locales/en_AU/address/city.ts 100.00% <100.00%> (ø)
src/locales/en_AU/address/index.ts 100.00% <100.00%> (ø)
... and 30 more

@ST-DDT ST-DDT changed the title feat(address): use fake pattern in city feat(address): use localized fake pattern in city May 12, 2022
@pkuczynski
Copy link
Member

What's the value of having both methods city and cityName? As a first time user I would feel a bit confused...

@ST-DDT
Copy link
Member Author

ST-DDT commented May 12, 2022

For now, mainly legacy reasons, but for some usecases it might be required to have actual city names instead of generated ones.
Maybe we could (later) merge them together using a generated: boolean = true flag.

@ST-DDT
Copy link
Member Author

ST-DDT commented May 12, 2022

(I do plan to deprecate the cityPrefix and citySuffix methods at some point though)

@pkuczynski
Copy link
Member

Following the comment in the other PR we both agreed that it would be better to have data close to reality (zodiac I think) so I think city should always return real city names if we have localization data and only generated ones when we don't...

@ST-DDT
Copy link
Member Author

ST-DDT commented May 12, 2022

Following the comment in the other PR we both agreed that it would be better to have data close to reality (zodiac I think) so I think city should always return real city names if we have localization data and only generated ones when we don't...

That sounds like a reasonable suggestion. I would like to do that in a separate PR though, as these mass locale changes get confusing/unreviewable really fast. Should I also remove the city (pattern) files, if there are actual city name files?

@pkuczynski
Copy link
Member

That sounds like a reasonable suggestion. I would like to do that in a separate PR though, as these mass locale changes get confusing/unreviewable really fast.

Make sense :)

Should I also remove the city (pattern) files, if there are actual city name files?

Yes. And deprecate cityName...

@ST-DDT ST-DDT merged commit 7373a22 into main May 16, 2022
@ST-DDT ST-DDT deleted the feat/city-use-fake-pattern branch May 16, 2022 07:41
@xDivisionByZerox xDivisionByZerox added the m: location Something is referring to the location module label Aug 26, 2022
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 c: locale Permutes locale definitions 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.

None yet

4 participants