feat(locale): add directions and directions abbr to pl#1225
feat(locale): add directions and directions abbr to pl#1225Shinigami92 merged 8 commits intofaker-js:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
+ Coverage 99.62% 99.63% +0.01%
==========================================
Files 2154 2156 +2
Lines 239914 239940 +26
Branches 1003 1008 +5
==========================================
+ Hits 239015 239066 +51
+ Misses 878 853 -25
Partials 21 21
|
ejcheng
left a comment
There was a problem hiding this comment.
Translations verified using Google Translate, lgtm
ST-DDT
left a comment
There was a problem hiding this comment.
The order of the elements is somewhat important:
faker/src/modules/address/index.ts
Lines 411 to 453 in 04c9d36
0-3 => cardinal direction
4-7 => ordinal direction
|
Wow! I would never expect this and it's really hard to catch on the review. It would be way better if TS could catch it automatically... Should we change the definition to look more like: interface Directions {
cardinal: string[]
ordinal: string[] // or intercardinal ?
secondaryIntercardinal?: string[]
}
type DirectionType = keyof DirectionsAnd then both Additionally, I think it makes sense to simplify the usage and change direction ({ abbr: boolean, type: DirectionType, types: DirectionType[]})
// @examples
faker.address.direction() // 'north'
faker.address.direction({ abbr: true }) // 'N'
faker.address.direction({ type: 'ordinal' }) // 'north west'
faker.address.direction({ abbr: true, type: 'ordinal' }) // 'NW'
faker.address.direction({ abbr: true, types: ['ordinal', 'secondaryIntercardinal'] }) // 'NNW'And then deprecate the other methods? We might also skip |
|
@pkuczynski Please create a separate issue for that. |
Sure! #1228 |
|
@hankucz Could you please restore the original order? (N,E,S,W, NE, SE, SW, NW) |
|
@ST-DDT original order restored |
No description provided.