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

fix: ASCII-fy domainWord() #1520

Merged
merged 6 commits into from
Dec 13, 2022
Merged

fix: ASCII-fy domainWord() #1520

merged 6 commits into from
Dec 13, 2022

Conversation

schw4rzlicht
Copy link
Contributor

@schw4rzlicht schw4rzlicht commented Nov 4, 2022

The en locale includes the word "jalapeño" which is the only one with a special char. Imo this is inconsistent and it leads to other problems as well, see #1105 (comment) for example.

This PR removes the word from the locale.

This PR uses faker.helpers.slugify() to remove non-ASCII characters from domainWord().

@schw4rzlicht schw4rzlicht requested a review from a team as a code owner November 4, 2022 15:49
@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2022

AFAICT: jalapeño is a proper English word, isn't it?
For #1105 I think we should rather ansify these characters.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: locale Permutes locale definitions labels Nov 4, 2022
@schw4rzlicht
Copy link
Contributor Author

AFAICT: jalapeño is a proper English word, isn't it?
For #1105 I think we should rather ansify these characters.

Yes, might make more sense. I altered the Regex that filteres illegal characters in Internet.domainWord(), maybe that's what helps?

@ST-DDT ST-DDT mentioned this pull request Nov 7, 2022
10 tasks
@matthewmayer
Copy link
Contributor

matthewmayer commented Nov 24, 2022

if #1554 gets merged then you would be able to use the new version of faker.helpers.slugify to turn jalapeño to jalapeno automatically in domainWord (as domainWord seems to be a slight variant of slugify really).

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #1520 (777ded6) into next (b2a72bc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1520      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files        2221     2221              
  Lines      240110   240108       -2     
  Branches     1051     1049       -2     
==========================================
- Hits       239248   239236      -12     
- Misses        841      851      +10     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/internet/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 84.05% <0.00%> (-2.71%) ⬇️

@schw4rzlicht
Copy link
Contributor Author

schw4rzlicht commented Dec 6, 2022

if #1554 gets merged then you would be able to use the new version of faker.helpers.slugify to turn jalapeño to jalapeno automatically in domainWord (as domainWord seems to be a slight variant of slugify really).

From what I've seen this is right.

But that would mean:

  • in order to fix that our team has to change the usage of faker for ~2000 test cases. And I don't think we're alone with this.
  • we cannot call faker.internet.domainWord()/faker.internet.domainName()/faker.internet.url() without using slugify() together with it, which imo is counter intuitive, not nice to read and will definitely break in the future again, as at some point someone will forget about it
  • it doesn't solve the underlying issue with domainWord()

As I said here:

From what I understand, not all TLDs are even accepting internationalized domain names (wiki), so I think it is out of scope for faker to determine which are and keep track of that. Imo, domain words should just not include non-ASCII chars to keep it simple.

Additionally, internationalized domain names are converted to ASCII compatible encoding on the client when using them. They simply don't exist when building servers.

Btw, I noticed I forgot to re-add jalapeño after refactoring the regex of domainWord(). Will do that once our discussion here moves forward :)

@Shinigami92
Copy link
Member

@schw4rzlicht I highly assume he meant using it inside faker in this PR, not in your code to wrap faker calls.

@matthewmayer
Copy link
Contributor

right could you change domainWord to just be something like

domainWord(): string {
    return this.faker.helpers
      .slugify(`${this.faker.word.adjective()}-${this.faker.word.noun()}`)
      .toLowerCase();
  }

@schw4rzlicht
Copy link
Contributor Author

schw4rzlicht commented Dec 6, 2022

@schw4rzlicht I highly assume he meant using it inside faker in this PR, not in your code to wrap faker calls.

Ah alright, misunderstood that, sorry 🤦

Thanks @matthewmayer, I will update the PR today or tomorrow :)

EDIT: Wasn't able to do it yet, too much work on other projects. Will do asap though.

@schw4rzlicht schw4rzlicht changed the title chore: remove "jalapeño" from en locale fix: ASCII-fy domainWord() Dec 12, 2022
src/modules/internet/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Dec 12, 2022
@ST-DDT ST-DDT requested review from matthewmayer and a team December 12, 2022 15:36
@ST-DDT ST-DDT merged commit cf764b9 into faker-js:next Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants