feat(helpers): add fromRegExp method

[wooneusean]

Fixes #1359

Example:

faker.helpers.regexpStyleStringParse('Test[a-d1-4]{2,4}'); // 'Testda3'
faker.helpers.regexpStyleStringParse('Test[^a-zA-Z0-8]{2}'); // 'Test99'
faker.helpers.regexpStyleStringParse('Test[-a-z]'); // Test-

[codecov]

Codecov Report

Merging #1569 (6360860) into next (2a4f137) will decrease coverage by 0.01%.
The diff coverage is 98.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1569      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2450     2450              
  Lines      239453   239748     +295     
  Branches     1226     1272      +46     
==========================================
+ Hits       238559   238847     +288     
- Misses        872      879       +7     
  Partials       22       22              

Impacted Files	Coverage Δ
src/modules/helpers/index.ts	99.03% <98.30%> (-0.21%)	⬇️

... and 1 file with indirect coverage changes

[ST-DDT]

Looks good to me.

However, we have to decide whether we want to introduce this as a new method (e.g. faker.string.forRegex) because it changes the behavior of the method.

[Shinigami92]

Yeah I'm with @ST-DDT, but I would maybe name it string.fromRegExp
from instead of for because it relates to the input parameter
and RegExp because that is how the class is named in JS

[ST-DDT]

and RegExp because that is how the class is named in JS

Good idea, then it should accept actual Regexp types as well. ( pattern: RegExp | string)

if (pattern instanceof RegExp) {
    pattern = pattern.toString();
}

it should also be prepared for regexp options such as case (in)sensitive.

[wooneusean]

Good idea, then it should accept actual Regexp types as well. ( pattern: RegExp | string)

Looking around online, I came across another library called RandExp.js that does exactly this. We can just use this library to facilitate that functionality. What do you guys think?

[Shinigami92]

@faker-js/faker is a dependency-free project right now, just to support this for a function that is used by <1% of our users might be really overkill
I would then more be of the favor of deprecating the function and log a warning to directly use their solution
But our current solution depends on this.faker.datatype.number-call and therefore depends on a deterministic seed
I think the RandExp.js solution does not provide that, does it?

[wooneusean]

But our current solution depends on this.faker.datatype.number-call and therefore depends on a deterministic seed
I think the RandExp.js solution does not provide that, does it?

Hmm you have a point. I'm new to open source, so tell me if this sounds crazy or not: I copy their implementation and use calls to faker's random number generation. That way we can have their functionality, and also keep the deterministic nature of the function.

This also sounds like overkill but its more realistic than writing one from scratch, that may have errors and bugs all over the place.

[ST-DDT]

IMO we should use the current implementation as a basic implementation. We can add a comment in the jsdocs that for complete regex support the users can use that library and explain how you can use it with faker (e.g. randexp.randInt = (min, max) => faker.datatype.number({min, max})) but not much more.

EDIT: That library is no longer maintained, so I don't think we should explicitly link it.

[Shinigami92]

For now my first thought would that this is definitely to much code (in terms of loc)
But in a later version of faker > 8.0 we want to restructure the project and move every single function to it's own file
Then it would not be anymore such a huge problem as everything is just one file that depends to this regex things

IMO even the current PR looks really long for just one function

Not sure what I can suggest you now to proceed or maybe just wait, but this could take months 😕

[wooneusean]

I guess there's no rush right? As you mentioned <1% of users probably use this functionality anyways. For now, I'll just wait 😐

[ST-DDT]

Sorry for the long wait.

Team decision

• For now we don't change the existing method
• Instead we will add a new method string.fromRegExp(string | RegExp): string
• We will create a separate issue regarding deprecation of the old method

[wooneusean]

Alright noted.

This seems like it has become a more complex problem as tokenising RegEx is needed to properly generate correct text (according to the previously mentioned library). If I were to try to implement it it might be a huge change.

Whats the course of action now?

[ST-DDT]

Could you please move the marked code to a separate method/move it to the already extracted method as well?