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(locale): use null as not applicable #2078

Merged
merged 7 commits into from
Apr 23, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 21, 2023

Created based on:

I mean that's literally what null is for. From the MDN Docs:

The null value represents the intentional absence of any object value.

Instead of relying on magic values such as [], {}, '', ... we should use null as explicitly absent values.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs c: locale Permutes locale definitions labels Apr 21, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Apr 21, 2023
@ST-DDT ST-DDT requested review from a team April 21, 2023 20:34
@ST-DDT ST-DDT self-assigned this Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #2078 (f60833d) into next (f795269) will increase coverage by 0.00%.
The diff coverage is 55.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2078   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2538     2538           
  Lines      242656   242636   -20     
  Branches     1298     1300    +2     
=======================================
- Hits       241669   241659   -10     
+ Misses        960      950   -10     
  Partials       27       27           
Impacted Files Coverage Δ
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/definitions/person.ts 0.00% <0.00%> (ø)
src/locales/az/company/suffix.ts 100.00% <100.00%> (ø)
src/locales/az/location/state.ts 100.00% <100.00%> (ø)
src/locales/az/person/female_prefix.ts 100.00% <100.00%> (ø)
src/locales/az/person/male_prefix.ts 100.00% <100.00%> (ø)
src/locales/az/person/prefix.ts 100.00% <100.00%> (ø)
src/locales/az/person/suffix.ts 100.00% <100.00%> (ø)
src/locales/id_ID/person/female_prefix.ts 100.00% <100.00%> (ø)
src/locales/id_ID/person/male_prefix.ts 100.00% <100.00%> (ø)
... and 21 more

... and 2 files with indirect coverage changes

import-brain
import-brain previously approved these changes Apr 21, 2023
@ST-DDT ST-DDT requested a review from a team April 21, 2023 21:09
Shinigami92
Shinigami92 previously approved these changes Apr 21, 2023
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Apr 21, 2023

What is the expected behavior for:

const result = mergeLocale([
 { 
    location: { 
      city: null,
      country: null,
    },
  },
 { 
    location: { 
      city: ['cityA'],
    },
  },
 { 
    location: { 
      country: ['countyA'],
    },
  }
]);

console.log(result); // ?

Is there anything we need to take care of in merging locales and null values?

@Shinigami92
Copy link
Member

{ 
  location: { 
    city: null,
    country: null,
  },
}

🤔

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 21, 2023

 { 
    location: { 
      city: null,
      country: null,
    },
  }

I'll write a test for that tomorrow, if I don't have one of those yet.

@xDivisionByZerox
Copy link
Member

[...] if I don't have one of those yet.

Currently the word null does not exist in the merge-locale.spec.ts file.

@xDivisionByZerox xDivisionByZerox added needs test More tests are needed do NOT merge yet Do not merge this PR into the target branch yet labels Apr 21, 2023
@ST-DDT ST-DDT dismissed stale reviews from import-brain and Shinigami92 via ddc4b6a April 22, 2023 08:56
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 22, 2023

I added the missing test.

@ST-DDT ST-DDT removed needs test More tests are needed do NOT merge yet Do not merge this PR into the target branch yet labels Apr 22, 2023
@ST-DDT ST-DDT requested a review from a team April 22, 2023 09:02
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.

Thank you 👍

@ST-DDT ST-DDT merged commit acb9cf5 into next Apr 23, 2023
@ST-DDT ST-DDT deleted the refactor/locale/null-as-not-applicable branch April 23, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants