Skip to content

refactor(locale): use null as not applicable#2078

Merged
ST-DDT merged 7 commits intonextfrom
refactor/locale/null-as-not-applicable
Apr 23, 2023
Merged

refactor(locale): use null as not applicable#2078
ST-DDT merged 7 commits intonextfrom
refactor/locale/null-as-not-applicable

Conversation

@ST-DDT
Copy link
Copy Markdown
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
Copy Markdown

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

ejcheng
ejcheng 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
Copy Markdown
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
Copy Markdown
Member

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

🤔

@ST-DDT
Copy link
Copy Markdown
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
Copy Markdown
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 ejcheng and Shinigami92 via ddc4b6a April 22, 2023 08:56
@ST-DDT
Copy link
Copy Markdown
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 review from Shinigami92 and ejcheng April 22, 2023 09:02
@ST-DDT ST-DDT requested a review from a team April 22, 2023 09:02
Copy link
Copy Markdown
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 👍

Comment thread src/definitions/person.ts
@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