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.

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

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

Merged
merged 7 commits into from
Jan 30, 2022
Merged

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

merged 7 commits into from
Jan 30, 2022

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

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 damienwebdev and a team January 23, 2022 12:43
@Shinigami92
Copy link
Member

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 Shinigami92 and a team 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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