Skip to content

fix: adds regex to random.word to trim unwanted characters #115

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 58 commits into from
Closed

fix: adds regex to random.word to trim unwanted characters #115

wants to merge 58 commits into from

Conversation

prisis
Copy link
Contributor

@prisis prisis commented Jan 12, 2022

No description provided.

@prisis prisis self-assigned this Jan 12, 2022
@Shinigami92
Copy link
Member

Please fix this in based on the main branch, it has nothing to do with the ts migration
Also we might want to have a test

@Shinigami92 Shinigami92 added needs rebase There is a merge conflict has workaround Workaround provided or linked needs test More tests are needed labels Jan 13, 2022
@Shinigami92
Copy link
Member

Also folks could just do it manually filter it in their code for now, so I do not see it that high prio

@MilosPaunovic
Copy link
Contributor

Also folks could just do it manually filter it in their code for now, so I do not see it that high prio

Yeah, but, it's more elegant if we deal with it ourselves in the codebase 🙂
Anyway, this was one of the opened issues at the original repo, so, someone definitely encountered it.

@Shinigami92 Shinigami92 added do NOT merge yet Do not merge this PR into the target branch yet and removed needs rebase There is a merge conflict labels Jan 13, 2022
@Shinigami92 Shinigami92 self-requested a review January 13, 2022 11:11
@Shinigami92 Shinigami92 linked an issue Jan 13, 2022 that may be closed by this pull request
random.word() can return undesirable non-alpha characters #113
Closed
@Shinigami92 Shinigami92 changed the title fix(#113): Adds regex to random.word to trim unwanted characters fix: adds regex to random.word to trim unwanted characters Jan 13, 2022
@Shinigami92 Shinigami92 force-pushed the migrate-to-typescript branch 3 times, most recently from 1705b9d to 23e4498 Compare January 14, 2022 20:33
@damienwebdev
Copy link
Member

This needs a rebase against main.

@Shinigami92 Shinigami92 added needs rebase There is a merge conflict and removed do NOT merge yet Do not merge this PR into the target branch yet labels Jan 15, 2022
imrishabh18 and others added 9 commits January 15, 2022 11:46
@imrishabh18
docs: updated meta tags for rich text preview (faker-js#140)
56ccca9
@JessicaSachs
docs: an update from the faker team (faker-js#138)
8ab656b
@MohdImran001 @Shinigami92
chore: add files field in package.json (faker-js#173)
db4c018 
Co-authored-by: Shinigami <[email protected]>
@damienwebdev
chore: update license (faker-js#157)
2cc11e4
@ejcheng
fix: typo in update.md (faker-js#180)
4a69d6d
@ejcheng @Shinigami92 @fzn0x
docs: add yarn and pnpm install commands (faker-js#163)
6871f63 
* docs(index.md): add yarn command in index.md

Add yarn install command in the installation section of guide/index.md.

* fix: docs/guide/index.md suggestion

Co-authored-by: Shinigami <[email protected]>

* docs(readme.md): add yarn/pnpm install commands

Co-authored-by: fncolon <[email protected]>

Co-authored-by: Shinigami <[email protected]>
Co-authored-by: fncolon <[email protected]>
@antfu
chore: support dark mode for favicon (faker-js#195)
156b76f
@damienwebdev
chore: retain original license in its exact form (faker-js#185)
cd4695a
@Shinigami92
chore(docs): fix imports (faker-js#154)
a973ee1
@damienwebdev
Copy link
Member

damienwebdev commented Jan 17, 2022 •
edited
Loading

I'm not sure that this implementation is correct. It seems like we should be modifying the word dictionary of a locale to be more correct for that locale rather than relying on a regex.

@MilosPaunovic
Copy link
Contributor

@damienwebdev That approach might be problematic, as words are generated from multiple methods

faker/src/random.ts

Line 169 in a973ee1

const wordMethods = [

And some of which, like countries, just have the parenthesis, mostly seen here:
https://github.com/faker-js/faker/blob/main/lib/locales/en/address/country.js

We could go through all the locales, but removing unwanted characters might change meaning of the word or make it incomprehensible.

For the reasons above, my vote goes to the regex approach.

@prisis prisis force-pushed the feature/sanitize-random-word branch from 54805d4 to 55e93f9 Compare January 18, 2022 12:12
@prisis prisis mentioned this pull request Jan 18, 2022
Closed
@prisis prisis closed this Jan 18, 2022
@prisis prisis deleted the feature/sanitize-random-word branch January 18, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shinigami92 Shinigami92 Awaiting requested review from Shinigami92

Assignees

@prisis prisis

Labels
has workaround Workaround provided or linked needs rebase There is a merge conflict needs test More tests are needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

random.word() can return undesirable non-alpha characters