-
-
Notifications
You must be signed in to change notification settings - Fork 933
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(locale): add directions and directions abbr to pl #1225
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translations verified using Google Translate, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of the elements is somewhat important:
faker/src/modules/address/index.ts
Lines 411 to 453 in 04c9d36
/** | |
* Returns a random cardinal direction (north, east, south, west). | |
* | |
* @param useAbbr If true this will return abbreviated directions (N, E, etc). | |
* Otherwise this will return the long name. Defaults to `false`. | |
* | |
* @example | |
* faker.address.cardinalDirection() // 'North' | |
* faker.address.cardinalDirection(false) // 'South' | |
* faker.address.cardinalDirection(true) // 'N' | |
*/ | |
cardinalDirection(useAbbr: boolean = false): string { | |
if (!useAbbr) { | |
return this.faker.helpers.arrayElement( | |
this.faker.definitions.address.direction.slice(0, 4) | |
); | |
} | |
return this.faker.helpers.arrayElement( | |
this.faker.definitions.address.direction_abbr.slice(0, 4) | |
); | |
} | |
/** | |
* Returns a random ordinal direction (northwest, southeast, etc). | |
* | |
* @param useAbbr If true this will return abbreviated directions (NW, SE, etc). | |
* Otherwise this will return the long name. Defaults to `false`. | |
* | |
* @example | |
* faker.address.ordinalDirection() // 'Northeast' | |
* faker.address.ordinalDirection(false) // 'Northwest' | |
* faker.address.ordinalDirection(true) // 'NE' | |
*/ | |
ordinalDirection(useAbbr: boolean = false): string { | |
if (!useAbbr) { | |
return this.faker.helpers.arrayElement( | |
this.faker.definitions.address.direction.slice(4, 8) | |
); | |
} | |
return this.faker.helpers.arrayElement( | |
this.faker.definitions.address.direction_abbr.slice(4, 8) | |
); | |
} |
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 Directions And 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.