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

refactor(person)!: flatten jobs definitions #2505

Merged
merged 11 commits into from
Feb 27, 2024

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Oct 27, 2023

Currently the definitions for jobArea, jobDescriptor and jobType methods
are nested inside a title.ts file. This causes undesired errors (ref #2503 ) when some
of the job definitions are supplied but not others (e.g. in fr, ur, ar).

This removes the special cases, and just makes job_area, job_descriptor
and job_title simple string arrays like other definitions.

The bulk locale changes were done with a simple script followed by linting.

This changes definition files but we generally don't consider those breaking changes when only accessed via faker.helpers.fake? Could be deferred until 9.0 if considered breaking.

const { allLocales } = require('@faker-js/faker');
const fs = require('fs');
const keys = Object.keys(allLocales);
for (let key of keys) {
  if (!['base'].includes(key)) {
    const localFaker = allLocales[key];
    if (localFaker.person.title) {
      const title = localFaker.person.title;
      if (title.level) {
        write('job_area', title.level, key);
      }
      if (title.descriptor) {
        write('job_descriptor', title.descriptor, key);
      }
      if (title.job) {
        write('job_type', title.job, key);
      }
      try {
        fs.rmSync("src/locales/" + key + "/person/title.ts")
      } catch (e) {}
    }
  }
}

function write(method, definitions, locale) {
  console.log(method, locale, definitions.length);
  fs.writeFileSync(
    'src/locales/' + locale + '/person/' + method + '.ts',
    'export default ' + JSON.stringify(definitions, null, 2)
  );
}

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: Patch coverage is 99.65428% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 99.56%. Comparing base (682a427) to head (3943cc9).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2505      +/-   ##
==========================================
- Coverage   99.57%   99.56%   -0.01%     
==========================================
  Files        2815     2846      +31     
  Lines      249845   249892      +47     
  Branches     1064     1055       -9     
==========================================
+ Hits       248777   248815      +38     
+ Misses       1068     1048      -20     
- Partials        0       29      +29     
Files Coverage Δ
src/index.ts 100.00% <ø> (ø)
src/locales/ar/person/index.ts 100.00% <100.00%> (ø)
src/locales/ar/person/job_type.ts 100.00% <100.00%> (ø)
src/locales/el/person/index.ts 100.00% <100.00%> (ø)
src/locales/el/person/job_area.ts 100.00% <100.00%> (ø)
src/locales/el/person/job_descriptor.ts 100.00% <100.00%> (ø)
src/locales/el/person/job_type.ts 100.00% <100.00%> (ø)
src/locales/en/person/index.ts 100.00% <100.00%> (ø)
src/locales/en/person/job_area.ts 100.00% <100.00%> (ø)
src/locales/en/person/job_descriptor.ts 100.00% <100.00%> (ø)
... and 59 more

... and 28 files with indirect coverage changes

@matthewmayer matthewmayer self-assigned this Oct 27, 2023
@matthewmayer matthewmayer added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: person Something is referring to the person module labels Oct 27, 2023
@matthewmayer matthewmayer added this to the vAnytime milestone Oct 27, 2023
@matthewmayer matthewmayer marked this pull request as ready for review October 27, 2023 13:43
@matthewmayer matthewmayer requested a review from a team as a code owner October 27, 2023 13:43
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

Keep in mind that the prebuild faker instances contain more locale data than the actual locale. You should itterate over the local objects instead in your generation script. This should also reduce the diff significantly (due to current false positives).

src/index.ts Show resolved Hide resolved
src/locales/af_ZA/person/index.ts Outdated Show resolved Hide resolved
@matthewmayer matthewmayer added the breaking change Cannot be merged when next version is not a major release label Oct 27, 2023
@matthewmayer matthewmayer modified the milestones: vAnytime, v9.0 Oct 27, 2023
@matthewmayer
Copy link
Contributor Author

Keep in mind that the prebuild faker instances contain more locale data than the actual locale. You should itterate over the local objects instead in your generation script. This should also reduce the diff significantly (due to current false positives).

My bad I thought rawDefinitions was for that. Will fix for the locales that were previously inheriting.

@matthewmayer
Copy link
Contributor Author

I noticed that the sk, cs_CZ, he, pl data are just untranslated copies of en. Would it make sense to remove them as part of this PR?

@ST-DDT
Copy link
Member

ST-DDT commented Oct 27, 2023

I noticed that the sk, cs_CZ, he, pl data are just untranslated copies of en. Would it make sense to remove them as part of this PR?

If the files are just plain copies, we can remove them now and the users wouldnt get any breakages. IMO We dont have to wait for v9 with that.

@ST-DDT
Copy link
Member

ST-DDT commented Oct 30, 2023

Please fix the "merge conflicts" (although I think it will most likely merge locally without issues).

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Oct 30, 2023
@matthewmayer
Copy link
Contributor Author

if this won't be merged until v9 is it worth bothering to fix merge conflicts until we are ready to start on v9?

@ST-DDT
Copy link
Member

ST-DDT commented Oct 30, 2023

No, there is no need to fix them now. I just left a comment to go along with the needs rebase label for improved visibility.

@matthewmayer
Copy link
Contributor Author

OK :D 90% of my notifications for this project are about merge commits, so i wont bother adding to the volume for now!

@ST-DDT
Copy link
Member

ST-DDT commented Oct 30, 2023

OK :D 90% of my notifications for this project are about merge commits, so i wont bother adding to the volume for now!

I wished there was a way to turn them off (or get them merged 😉)...

# Conflicts:
#	src/locales/en/person/index.ts
#	src/locales/es/person/index.ts
#	src/locales/fr/person/index.ts
#	src/locales/fr/person/title.ts
#	src/modules/person/index.ts
#	test/all-functional.spec.ts
@matthewmayer matthewmayer requested a review from a team January 6, 2024 17:27
@xDivisionByZerox xDivisionByZerox removed the needs rebase There is a merge conflict label Jan 6, 2024
@ST-DDT ST-DDT changed the title refactor(person): flatten jobs definitions refactor(person)!: flatten jobs definitions Feb 8, 2024
ST-DDT
ST-DDT previously approved these changes Feb 8, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Feb 8, 2024

Ready for review

@ST-DDT ST-DDT requested review from a team February 8, 2024 19:25
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Feb 11, 2024
Shinigami92
Shinigami92 previously approved these changes Feb 27, 2024
@ST-DDT ST-DDT dismissed stale reviews from Shinigami92, xDivisionByZerox, and themself via 3943cc9 February 27, 2024 19:35
@ST-DDT
Copy link
Member

ST-DDT commented Feb 27, 2024

3 approving reviews. Enabling auto-merge.

@ST-DDT ST-DDT enabled auto-merge (squash) February 27, 2024 19:37
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: person Something is referring to the person module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants