Skip to content

Internet provider: re-implement url, add webdomain method #841

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
Merged
merged 1 commit into from
May 27, 2023
Merged

Internet provider: re-implement url, add webdomain method #841

merged 1 commit into from
May 27, 2023

Conversation

kingthorin
Copy link
Collaborator

@kingthorin kingthorin commented May 23, 2023 •
edited
Loading

  • Internet.java > Re-implement url. Have it leverage the new webdomain method (which contains the original url impl). The new url implementation creates a fully formed URL with random elements.
  • InternetTest.java > Updated to test the new url and webdomain methods.

Fixes #840

@what-the-diff
Copy link

what-the-diff bot commented May 23, 2023 •
edited by kingthorin
Loading

PR Summary

  • Re-implemented url() method
    The old method url() has been is now replaced by updated methods with greater options and accuracy to it's name.

  • New method webdomain() added
    A new method webdomain() has been introduced to provide the functionality previously provided by url() which was simply a random domain in the form www.example.com.

Edit: by @kingthorin

@kingthorin kingthorin force-pushed the inet-url-host branch 2 times, most recently from 18e411e to 6972ad1 Compare May 23, 2023 00:25
@kingthorin
Copy link
Collaborator Author

Here's a sample:

https://gwyn.friesen:k23412dxo7bc8@www.genie-parisian.biz:47439/?aut=magnam&voluptatum=deserunt#assumenda
https://www.jorge-mcglynn.info:19707/?reiciendis=aut&pariatur=sequi#ut
https://www.piper-kilback.net:1013/et?rerum=beatae&est=magni#qui
https://klara.okon:754vazl1z7n1f10@www.jacquiline-hintz.com:31519/?corrupti=consectetur&nihil=minima
http://www.mike-watsica.com/#consequatur
https://www.candyce-goldner.biz/omnis?vero=molestiae&dolorem=rerum#enim
https://kraig.auer:23j7f9i03n9d95@www.bert-rogahn.org/occaecati/eaconsequatur?explicabo=placeat&unde=aut#commodi
http://www.verdell-bogisich.net/ut/similique?error=magnam&perferendis=impedit
https://www.thao-upton.io/qui/liberoenim?consequuntur=earum&omnis=eveniet
http://www.sommer-nienow.info:8069/

@kingthorin kingthorin changed the title internet provider: deprecate url, add uri and hostname methods internet provider: deprecate url, add weburl and webdomain methods May 24, 2023
@kingthorin kingthorin force-pushed the inet-url-host branch 2 times, most recently from 91fa67a to 639fadd Compare May 24, 2023 01:54
@kingthorin
Copy link
Collaborator Author

kingthorin commented May 24, 2023 •
edited
Loading

@bodiam happy with the current implementation?

(I know it only needs one review, but wanted to give you a chance since you'd provided feedback earlier.)

@bodiam
Copy link
Contributor

bodiam commented May 24, 2023

Thanks for your work here. It's not entirely what I expected / had in mind. I don't want to deprecate the url() method, I rather have it return sensible urls, and the same for urn().

The current methods seem not like great replacements, I don't think I've ever seen a username and password in a web url (in ftp urls perhaps), and I don't think they would provide useful generators like this.

@kingthorin
Copy link
Collaborator Author

kingthorin commented May 24, 2023 •
edited
Loading

I don't mind dropping the credential part, no big deal.

As for completely re-working url() I think that's a mistake. I mean I guess we are going to 2.0.0 so a breaking change is fine (semver wise), but we need to be sure this is a "headline" in the release notes if we go that way. Also as pointed out earlier URL can be any number of things not just web.

@bodiam
Copy link
Contributor

bodiam commented May 24, 2023

I don't think returning a url from a url method is that unexpected. But launching 2.0 with already deprecated methods in it, that's not ideal.

@kingthorin
Copy link
Collaborator Author

Okay. I'll trim the cred part and rename weburl() -> url()

@kingthorin
internet provider: re-implement url, add webdomain method
c4d22d8 
- Internet.java > Re-implement url. Have it leverage the
new webdomain method (which contains the original url impl). The new url
implementation creates a fully formed URL with random elements.
- InternetTest.java > Updated to test the new url and webdomain methods.

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@kingthorin kingthorin changed the title internet provider: deprecate url, add weburl and webdomain methods nternet provider: re-implement url, add webdomain method May 24, 2023
@kingthorin
Copy link
Collaborator Author

@bodiam Done? 😀

@kingthorin kingthorin changed the title nternet provider: re-implement url, add webdomain method Internet provider: re-implement url, add webdomain method May 24, 2023
bodiam
bodiam reviewed May 27, 2023
View reviewed changes
src/main/java/net/datafaker/providers/base/Internet.java
public String url() {
final byte[] bts = faker.random().nextRandomBytes(6);
return url(bts[0] % 2 == 0, bts[1] % 2 == 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny formatting thing here, but all good.

@bodiam
Copy link
Contributor

bodiam commented May 27, 2023

LGTM!

@bodiam bodiam merged commit e2ce76d into datafaker-net:main May 27, 2023
@kingthorin kingthorin deleted the inet-url-host branch May 27, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bodiam bodiam bodiam left review comments

@snuyanzin snuyanzin snuyanzin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

faker.internet().url() creates the hostname only without the scheme
3 participants
@kingthorin @bodiam @snuyanzin