refactor(random.arrayElement): remove default parameter

[xDivisionByZerox]

Fixes #581.

[xDivisionByZerox]

Should this change also apply to arrayElements?

[xDivisionByZerox]

Ok cool of course I forgot to look at the test cases on such a method.
Will provide updates for test files in near future.

[xDivisionByZerox]

address.streetPrefix implementation:

faker/src/address.ts

Lines 234 to 238 in 60d3cc5

	streetPrefix(): string {
	return this.faker.random.arrayElement(
	this.faker.definitions.address.street_prefix
	);
	}

Meaning that if a local doesn't export a street_prefix it will throw an error (example 'de').
To fix this we would need to make all definition maps required (since its not only address.streetPrefix that breaks from this).

[xDivisionByZerox]

How to handle this?
@ST-DDT @Shinigami92

[ST-DDT]

Meaning that if a local doesn't export a street_prefix it will throw an error (example 'de').

It will only fail, if both the selected and the fallback locale don't have that value.
IMO throwing an error is still better than returning abc randomly.
That way you at least notice, that it's not supported.

[ST-DDT]

Please also update random.arrayElements() accordingly + fix the tests.

E.g. by changing:

return (
      this.faker.random.arrayElement(wordList) ||
      this.faker.random.arrayElement(this.faker.definitions.word.adjective)
    );

to

return (
      wordList.length > 0 ? this.faker.random.arrayElement(wordList) : this.faker.random.arrayElement(this.faker.definitions.word.adjective)
    );

Maybe add a check that checks the array before calling arrayElement and throws an error that contains a the method/definition that caused it.

---

For word.ts it might be useful to extract the entire logic into a separate private method:

adjective(length?: number): string {
    return selectOneWord(length, this.faker.definitions.word.adjective);
}

private selectOneWord(length?: number, words: string[]): string {
  let wordList = words;
    if (length) {
      wordList = words.filter((word) => word.length == length);
      if (!wordList.length) {
        wordList = words;
      }
    }

    return this.faker.random.arrayElement(words);
}

[xDivisionByZerox]

Maybe add a check that checks the array before calling arrayElement and throws an error that contains a the method/definition that caused it.

Can I expect intern-provided arrays (like this.faker.definitions.word.adjective) to be provided correctly or check them as well?

[ST-DDT]

Theoretically we don't have an intern-provided array, en is just a fallback, but you can specify a different one (and might be sensible to do so, if you don't use a language with latin characters).

Maybe we should use special syntax for the definitions to make this implicitly?

• this.faker.definitions.word.adjective throw if the entry is missing
• this.faker.extensions.word.adjective return undefined

(both would use the same "data source")

This kind of relates to #265 and the more I think of this, the more it feels like a big change to be made in v7.

[Shinigami92]

@xDivisionByZerox In preparation of starting to review some v6.1 PRs, could you rebase this one and fix conflicts?

[xDivisionByZerox]

Rebase is done and conflicts resolved. Haven't had time for the test and probably won't have this week either.

[Shinigami92]

Due to how we handle our "module" classes, it's not possible to define private methods in them
So you need to extract selectOneWord out of the class and define it above as a raw function (that is not exported!)
An example for that can be found here: https://github.com/faker-js/faker/pull/644/files#diff-f2b24a1df2ffea4071c80b9948ecf7ed59397468799db7ee1ec376b3c9740b06R17

Also this PR needs a rebase 🙂

[xDivisionByZerox]

Not really since the problem is the same I stated 1 month ago.
Or should I change the address.streetPrefix to expect an error?

[Shinigami92]

address.streetPrefix breaks our tests, since there are no locales in en for street_prefix

So yeah, but as I understand this PR: This is THE bug we want to fix!
So do we need to throw errors in this case?
@ST-DDT I would like you to decide it how to proceed

[xDivisionByZerox]

I'll change the test to expect an Error and add a comment to expect a real value when locales were included.

[xDivisionByZerox]

@ST-DDT Can you give insight into how the BROKEN_LOCALE_METHODS constant in all_functional.spec.ts works? Do I need to add the modules that don't work with en and provide an alternative local or does this object contain all functions not working in a specific local?

[damienwebdev]

My opinion is that there's a non-trivial chance that this is breaking for someone. If we release this as v6.1.2 or v6.2 we would be breaking the semver contract.

[ST-DDT]

My opinion is that there's a non-trivial chance that this is breaking for someone. If we release this as v6.1.2 or v6.2 we would be breaking the semver contract.

I agree. We should move this to v7.

[ST-DDT]

@ST-DDT Can you give insight into how the BROKEN_LOCALE_METHODS constant in all_functional.spec.ts works? Do I need to add the modules that don't work with en and provide an alternative local or does this object contain all functions not working in a specific local?

Yes, if effectively contains all tests that are disabled for a certain locale.

[xDivisionByZerox]

@ST-DDT Can you give insight into how the BROKEN_LOCALE_METHODS constant in all_functional.spec.ts works? Do I need to add the modules that don't work with en and provide an alternative local or does this object contain all functions not working in a specific local?

Yes, if effectively contains all tests that are disabled for a certain locale.

Well then this doesn't work I guess. If you have a look at the log files from Build & Unit Test node-16, ubuntu-latest/7_Test.txt line 22345:

2022-04-01T15:28:45.2251825Z  × test/all_functional.spec.ts > faker.fake functional tests > ru > name > prefix()
2022-04-01T15:28:45.2252117Z    → Cannot get random element from empty array.

Even tho we have the following in all_functions.spec.ts:

const BROKEN_LOCALE_METHODS = {
    ...,
    name: {
        prefix: [..., 'ru'],
        ...,
    },
    ...,
}

Or did I misunderstood you?

[xDivisionByZerox]

Or can I remove this now since the it.fails doesn't apply anymore since the function will now throw an error? But for my understanding it.fails should be the right why to go when a function throws an error :/

[ST-DDT]

The faker.fake method currently does not have an exclude handling, as it does not fail.
https://github.com/faker-js/faker/blob/main/test/all_functional.spec.ts#L88
https://github.com/faker-js/faker/blob/main/test/all_functional.spec.ts#L108

[ST-DDT]

@xDivisionByZerox Can you split the word improvements to its own PR?
AFAICT these changes are independent and could be merged earlier.

[xDivisionByZerox]

@xDivisionByZerox Can you split the word improvements to its own PR? AFAICT these changes are independent and could be merged earlier.

Well you yourself suggested this change in this PR. This logic is not required to change if this PR is not in place. So a separate PR should IMO be dismissed because it doesn't provide any meaningful architectural refactoring/performance boosting.

[xDivisionByZerox]

But yeah, I can provide a separate PR if you wish to merge this in 6.3.0.

[ST-DDT]

@xDivisionByZerox Can you split the word improvements to its own PR? AFAICT these changes are independent and could be merged earlier.

Well you yourself suggested this change in this PR. This logic is not required to change if this PR is not in place. So a separate PR should IMO be dismissed because it doesn't provide any meaningful architectural refactoring/performance boosting.

It simplifies the code by removing duplicate code. IMO that is worth enough.
(I plan to extend the length to a range at some point, so any simplifications are welcome.)

[xDivisionByZerox]

I will close this PR due to MASSIV changes in the file structure. I will provide a new PR with changes discussed in this PR.