fix(lorem): return word with maximum possible length when expected length is too large #1218
Conversation
…ed length is too large
Codecov Report
@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
- Coverage 99.62% 99.62% -0.01%
==========================================
Files 2153 2153
Lines 236521 236537 +16
Branches 979 979
==========================================
+ Hits 235643 235646 +3
- Misses 857 870 +13
Partials 21 21
|
ejcheng
added
c: bug
| properLengthWords = this.faker.definitions.lorem.words.filter((word) => { | ||
| if (word.length > maxLength) { | ||
| maxLength = word.length; | ||
| } | ||
|
|
||
| return word.length === length; | ||
| }); |
There was a problem hiding this comment.
Beside that this is very costy, you already used reduce in test file, but why not here?
There was a problem hiding this comment.
Filtering was already part of the implementation before with word.length === length. So this is no change. Adding calculating max would not increase the cost of filtering much. So don't think this is a major problem.
There was a problem hiding this comment.
Previously you ran only once through all words, now you do it twice
There was a problem hiding this comment.
It will only run a second time when the length is not satisfied. I don't see any other way of implementing this unless we want to throw, as @ST-DDT suggests, but I don't think it's a good idea.
@Shinigami92, any other suggestions welcome...
| @@ -18,20 +18,36 @@ export class Lorem { | |||
| * Generates a word of a specified length. | |||
| * | |||
| * @param length length of the word that should be returned. Defaults to a random length. | |||
| * If a word with the specified length does not exist return a word with maximum possible length. | |||
There was a problem hiding this comment.
This does not fit my expectations regarding this parameter.
If the length of the word doesn't matter for me, then I wouldn't specify it.
The very fact, that I specify a specific length expresses intend that I actually want a word of that particular length.
If I just want a very long word, I dig into the data pick a suitable length and use that (Or maybe use a range parameter to narrow it down).
So this method should throw an error, if it cannot satisfy the requested data or should have an optional param/option that allows for a "closest fit". Not necessarily the longest. e.g. requesting a word with length 1 should not return Rindfleischetikettierungsüberwachungsaufgabenübertragungsgesetz (which is the name of an actual German law until 2013) according to https://de.babbel.com/de/magazine/das-laengste-wort.
There was a problem hiding this comment.
This is not for general words, this is for lorem, which is the same for all languages (although it does change the alphabet for some, like jp).
I can change implementation to throw an error, but when I had a brief look, we don't do that in other places. For example, faker.word.* would return a word of any length, when length can not be satisfied: https://github.com/faker-js/faker/blob/main/src/modules/word/index.ts#L11
So if we want to stay consistent, that's fine for me. I can change it in any other place. The question is: should faker be strict or be as close to the expectation as possible?
There was a problem hiding this comment.
I now looked a bit deeper into the implementation and tried to think about what your proposed implementation does.
And I'm more and more on @ST-DDT side (comment above here) that this current proposed PR is not what I, @ST-DDT or a user would expect.
We might need to discuss this in a team meeting, but I would currently reject your proposed solution, sorry 🙁
There was a problem hiding this comment.
Yeah, let's discuss this at the meeting, as different functions behave differently. As @hankucz pointed out, faker.word.* would ignore length if it can not be satisfied. So if we want to throw an error, we would need to change the behavior of those other methods too, to stay consistent.
| @@ -18,20 +18,36 @@ export class Lorem { | |||
| * Generates a word of a specified length. | |||
| * | |||
| * @param length length of the word that should be returned. Defaults to a random length. | |||
| * If a word with the specified length does not exist return a word with maximum possible length. | |||
There was a problem hiding this comment.
I now looked a bit deeper into the implementation and tried to think about what your proposed implementation does.
And I'm more and more on @ST-DDT side (comment above here) that this current proposed PR is not what I, @ST-DDT or a user would expect.
We might need to discuss this in a team meeting, but I would currently reject your proposed solution, sorry 🙁
|
See here for our team decision: #1131 (comment) |
Shinigami92
changed the title
|
This PR is stale and does not match our team decision. |
Fixes #1131