faker
faker copied to clipboard
Simplify `address.direction`
Clear and concise description of the problem
There are 3 separate functions now direction
, cardinalDirection
and ordinalDirection
, which is not convenience to use. Also they depend on specific order of localisation, which is hard to verify when reviewing PR.
Suggested solution
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 directions
and directions_abbr
should use this type?
Additionally, I think it makes sense to simplify the usage and change direction
signature to:
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?
Alternative
We might also skip secondaryIntercardinal
...
Additional context
No response
The proposed signature confuses me a bit:
e.g. what will happen if you call faker.address.direction({ abbr: true, type: 'cardinal', types: ['ordinal'] })
I as a user calling this or reading this code would be totally confused what it could return
Yeah, good point! How about direction ({ abbr: boolean, types: DirectionType[] })
or direction ({ abbr?: boolean, type?: DirectionType, except?: DirectionType[]})
Both are better, lets discuss more on team meeting
But second proposed is also dangerous as you can call direction ({ type: ['ordinal'], except: ['ordinal'] })
and we would need to throw an error
That's correct. But I guess more predictable. And unlikely someone would do it anyway. So we have the following proposals on the table, just to summarise:
direction ({ abbr?: boolean, type?: DirectionType | DirectionType[] })
direction ({ abbr?: boolean, types?: DirectionType[] })
direction ({ abbr?: boolean, type?: DirectionType, except?: DirectionType[]})
I think I like the first most.
Team decision
Refactor our locales to match these:
interface Directions {
cardinal: string[]
ordinal: string[]
}
Merge the directions functions into a single method:
direction ({ abbr?: boolean = false, type?: DirectionType | DirectionType[] = ['cardinal', 'ordinal'] })
Throw if type
is an empty array.
Hello, I can take care of this one, please assign it to me
We already discussed this with @hankucz and she will look at it. Thank you @wael-fadlallah
Yeah, I'm already looking into it