Skip to content

fix: improve internet tests and fix bug in domain name generation#258

Merged
prisis merged 7 commits intofaker-js:mainfrom
ST-DDT:fix/internet/domain-name
Jan 30, 2022
Merged

fix: improve internet tests and fix bug in domain name generation#258
prisis merged 7 commits intofaker-js:mainfrom
ST-DDT:fix/internet/domain-name

Conversation

@ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Jan 22, 2022

Fixes #256

@ST-DDT ST-DDT requested a review from a team as a code owner January 22, 2022 10:34
@netlify
Copy link

netlify bot commented Jan 22, 2022 •
edited
Loading

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: 2317d2f

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61ebddbcf28d4f0008408095

😎 Browse the preview: https://deploy-preview-258--vigilant-wescoff-04e480.netlify.app

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 22, 2022 •
edited
Loading

@cduff Does your tooling consider https://full-jalapeã±o.info to be a valid url?
What about https://full-jalapeño.info?

Or more specifically should we return such values? (Other languages might have more of them)

faker/src/locales/en/word/noun.ts

Line 3252 in 60c9002

'jalapeño',

@ST-DDT ST-DDT mentioned this pull request Jan 22, 2022
Merged
@Shinigami92 Shinigami92 added the p: 2-high Fix main branch label Jan 22, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 22, 2022

If jalapeño is not a valid value for a URL, then we would need to normalize the URL
Maybe this is a good idea in general 🤔

damienwebdev
damienwebdev previously requested changes Jan 22, 2022
View reviewed changes
@cduff
Copy link

cduff commented Jan 23, 2022

@cduff Does your tooling consider https://full-jalapeã±o.info to be a valid url? What about https://full-jalapeño.info?

Or more specifically should we return such values? (Other languages might have more of them)

faker/src/locales/en/word/noun.ts

Line 3252 in 60c9002

'jalapeño',

I'm using https://www.npmjs.com/package/class-validator isUrl for the validation. It reports the two strings you specified above as valid.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 23, 2022

I also added the verification checks to the other internet module tests.

Shinigami92
Shinigami92 reviewed Jan 23, 2022
View reviewed changes
Shinigami92
Shinigami92 previously approved these changes Jan 23, 2022
View reviewed changes
@Shinigami92 Shinigami92 requested review from a team and damienwebdev January 23, 2022 12:43
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 23, 2022

We should find a better title for the PR now 😉

@ST-DDT ST-DDT changed the title fix(internet): domain names should not contain spaces test: improve internet tests and fix bug in domain name generation Jan 27, 2022
@ST-DDT ST-DDT requested a review from Shinigami92 January 27, 2022 13:12
Shinigami92
Shinigami92 previously approved these changes Jan 27, 2022
View reviewed changes
@Shinigami92 Shinigami92 requested a review from a team January 27, 2022 17:00
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Jan 28, 2022
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Jan 28, 2022
@ST-DDT ST-DDT requested a review from Shinigami92 January 28, 2022 13:06
@ST-DDT ST-DDT mentioned this pull request Jan 28, 2022
Closed
Shinigami92
Shinigami92 previously approved these changes Jan 28, 2022
View reviewed changes
@Shinigami92 Shinigami92 changed the title test: improve internet tests and fix bug in domain name generation fix: improve internet tests and fix bug in domain name generation Jan 28, 2022
@ST-DDT ST-DDT requested a review from a team January 28, 2022 15:27
@Shinigami92 Shinigami92 mentioned this pull request Jan 29, 2022
Merged
@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 29, 2022

If we want to use it for #303 or a release, then we should merge it now, otherwise we can wait a day for damien.

@ST-DDT ST-DDT dismissed stale reviews from prisis and Shinigami92 via d04f48c January 29, 2022 19:03
@ST-DDT ST-DDT requested review from a team and Shinigami92 January 29, 2022 19:06
Shinigami92
Shinigami92 previously approved these changes Jan 29, 2022
View reviewed changes
@ST-DDT ST-DDT requested a review from a team January 29, 2022 20:50
@Shinigami92 Shinigami92 requested a review from prisis January 30, 2022 11:48
@Shinigami92 Shinigami92 mentioned this pull request Jan 30, 2022
Closed
prisis
prisis reviewed Jan 30, 2022
View reviewed changes
prisis
prisis reviewed Jan 30, 2022
View reviewed changes
@prisis prisis merged commit c6f7aa9 into faker-js:main Jan 30, 2022
@ST-DDT ST-DDT deleted the fix/internet/domain-name branch January 30, 2022 20:45
@ST-DDT ST-DDT mentioned this pull request Feb 17, 2022
Closed
bmenant pushed a commit to bmenant/faker that referenced this pull request Mar 11, 2022
@ST-DDT @Shinigami92 @bmenant
fix: improve internet tests and fix bug in domain name generation (fa…
456740b 
…ker-js#258)

Co-authored-by: Shinigami <chrissi92@hotmail.de>
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
@ST-DDT @Shinigami92 @demipel8
fix: improve internet tests and fix bug in domain name generation (fa…
91c333d 
…ker-js#258)

Co-authored-by: Shinigami <chrissi92@hotmail.de>
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
@ST-DDT @Shinigami92 @demipel8
fix: improve internet tests and fix bug in domain name generation (fa…
45baa9c 
…ker-js#258)

Co-authored-by: Shinigami <chrissi92@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prisis prisis prisis approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@damienwebdev damienwebdev Awaiting requested review from damienwebdev

Assignees

@ST-DDT ST-DDT

Labels

c: test p: 2-high Fix main branch

Projects

None yet

Milestone

v6.0 - Ensure Project Foundation

Development

Successfully merging this pull request may close these issues.

faker.internet.url() returning invalid URLs with spaces

5 participants

@ST-DDT @Shinigami92 @cduff @prisis @damienwebdev