Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

. is not working in faker.helpers.fromRegExp #3302

Closed
matthewmayer opened this issue Dec 1, 2024 · 6 comments · Fixed by #3322
Closed

. is not working in faker.helpers.fromRegExp #3302

matthewmayer opened this issue Dec 1, 2024 · 6 comments · Fixed by #3322
Labels
c: bug Something isn't working m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent
Milestone

Comments

@matthewmayer
Copy link
Contributor

One of the examples in faker.helpers.fromRegexp is

faker.helpers.fromRegExp(/.{5}/) // '14(#B'

However this actually always returns '.....'

It's supposed to work like:
. => returns a wildcard ASCII character that can be any number, character or symbol. Can be combined with quantifiers as well.

found while testing #3301

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module labels Dec 1, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Dec 1, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Dec 1, 2024

I noticed that as well. Thanks for creating all these issues for those bugs.

@matthewmayer
Copy link
Contributor Author

It appears this has never worked

The snapshot tests have looked like this since the feature was added in #1569

https://github.com/faker-js/faker/blame/next/test/modules/__snapshots__/helpers.spec.ts.snap

exports[`helpers > 42 > fromRegExp > with wildcard character 1`] = `"."`;

exports[`helpers > 42 > fromRegExp > with wildcard character and min max quantifier 1`] = `".."`;

exports[`helpers > 42 > fromRegExp > with wildcard character and quantifier 1`] = `"..."`;

@matthewmayer
Copy link
Contributor Author

I'd suggest just removing the ". => returns a wildcard ASCII character that can be any number, character or symbol. Can be combined with quantifiers as well." from the docs, as well as that example and the snapshot tests

@ST-DDT
Copy link
Member

ST-DDT commented Dec 1, 2024

This is supposed to be regex.

@ST-DDT
Copy link
Member

ST-DDT commented Dec 3, 2024

Unfortunately, there are more issues with the current implementation, that aren't easily fixable.

  • wildcard character support (fixed for quantified)
  • case insensitive patterns (fixed for quantified)
  • already processed values cause re-processing. e.g. if a pattern produces functional regex symbols like [, then these symbols will be processed in the next loop run. (this is the reason why I cannot just fix non-quantified)

We might have to replace the entire (?) implementation

@matthewmayer
Copy link
Contributor Author

I don't think it's such a big deal if . Is not supported (as long as we clarify that in the documentation)

We should bear in mind RegExps are typically used to match a pattern. We are using them "backwards" - ie, generating a string which would match a given regexp. We'll likely only ever be able to support a subset of Regexp features, because Regexp is pretty complicated.

Although not matching a . wildcard would be a serious problem in a matching library, it doesn't seem that bad in this reversed generation scenario, because it's fairly rare to need a fake pattern which genuinely allows ANY character in a position. More commonly you'd need some subset like a-z, A-Z, 0-9, and some specific punctuation, and in that case it would be better to explicitly list them out. It wouldn't be very obvious what characters . might generate (is space allowed? è? ~? ฒ? 🕺🏽?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants