chore: add missing type on the system.commonFileName method #299
Conversation
src/system.ts
Outdated
| @@ -58,7 +58,7 @@ export class System { | |||
| * @method faker.system.commonFileName | |||
| * @param ext | |||
| */ | |||
| commonFileName(ext): string { | |||
| commonFileName(ext?: string | number): string { | |||
There was a problem hiding this comment.
question: where did you get these types from?
Currently it could be like any primitive like also boolean and bigint and such.
It's not clear to me where you did get this from
There was a problem hiding this comment.
Changing it to something string like it probably a good idea, but I it will be a breaking change.
There was a problem hiding this comment.
Maybe we can find something in the test suite how it wanted to be called.
I just currently say that it's not clear to me where the types are coming from
There was a problem hiding this comment.
I just guessed the types, as they were the only ones which made sense as extension really...
There was a problem hiding this comment.
number type represents any kind of number, so int, float, bigint right?
There was a problem hiding this comment.
I would go for the stricter string only version as used in DefinitelyTyped, because using unformatted numbers looks like an edge case for me.
E.g. you may use .001 but I have never used just .1 especially in random test data. But I would be okay with allowing numbers too, but then we should make sure that it results in a decent extension even for non integer values.
There was a problem hiding this comment.
On top of that 0 || 'something' => 'something'
There was a problem hiding this comment.
I assume this is the same for ''. Lets go for string then and document that '' is considered to be not set.
There was a problem hiding this comment.
I assume this is the same for
''. Lets got for string then and document that '' is considered to be not set.
Oh... yeah, you are right 😬
# Conflicts: # src/system.ts
No description provided.