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

Ports zig-uri to stdlib. #14207

Merged
merged 5 commits into from
Jan 7, 2023
Merged

Ports zig-uri to stdlib. #14207

merged 5 commits into from
Jan 7, 2023

Conversation

ikskuh
Copy link
Contributor

@ikskuh ikskuh commented Jan 5, 2023

Implements #14176

Copy link
Contributor

@wooster0 wooster0 left a comment

Choose a reason for hiding this comment

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

The TODO said "TODO: redo this implementation according to RFC 1738" and that RFC is the Uniform Resource Locators (URL) RFC.
The RFC 3986 implemented here is the Uniform Resource Identifier (URI) RFC, so I wonder if this should be renamed to std.URI.
There is a difference between a URL and a URI and the library itself seems to use that term too.

A URL is a specific type of URI, so a URI seems more general, so I think it would make sense to rename.

lib/std/Url.zig Outdated Show resolved Hide resolved
lib/std/Url.zig Outdated
Comment on lines 496 to 500
test "Special test" {
// This is for all of you code readers ♥
_ = try parse("https://www.youtube.com/watch?v=dQw4w9WgXcQ&feature=youtu.be&t=0");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test "Special test" {
// This is for all of you code readers ♥
_ = try parse("https://www.youtube.com/watch?v=dQw4w9WgXcQ&feature=youtu.be&t=0");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if i want to remove that. It's a very important URL

Choose a reason for hiding this comment

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

Rick-rolled by Nixpkgs! OH NO!

lib/std/Url.zig Outdated Show resolved Hide resolved
lib/std/Url.zig Outdated Show resolved Hide resolved
lib/std/Url.zig Outdated Show resolved Hide resolved
lib/std/Url.zig Outdated Show resolved Hide resolved
lib/std/Uri.zig Outdated Show resolved Hide resolved
@matu3ba
Copy link
Contributor

matu3ba commented Jan 5, 2023

Is this post still relevant https://perishablepress.com/stop-using-unsafe-characters-in-urls/#unsafe-characters?

A Dangerous Trend

Note: Things have changed a lot with Google and URI specification. So the following information may be
outdated, but the example serves to illustrate how developers should be mindful of specification when 
crafting URIs in applications.

Also:

Note that RFC 1738 is now obsolete, however the information remains useful in the general sense, and
is shared below for educational and reference purposes.

Did you also check the issue list on the standard? https://svn.apache.org/repos/asf/labs/webarch/trunk/uri/rev-2002/issues.html

@ikskuh
Copy link
Contributor Author

ikskuh commented Jan 5, 2023

@matu3ba i never took a look at the older uri specifications, i've made the implementation right after RFC 3986, so it should be up-to-date in those regards. I can double-check later tho

@ikskuh
Copy link
Contributor Author

ikskuh commented Jan 5, 2023

Is this post still relevant https://perishablepress.com/stop-using-unsafe-characters-in-urls/#unsafe-characters?

A Dangerous Trend

Note: Things have changed a lot with Google and URI specification. So the following information may be
outdated, but the example serves to illustrate how developers should be mindful of specification when
crafting URIs in applications.
Also:

Note that RFC 1738 is now obsolete, however the information remains useful in the general sense, and
is shared below for educational and reference purposes.
Did you also check the issue list on the standard? https://svn.apache.org/repos/asf/labs/webarch/trunk/uri/rev-2002/issues.html

I think those issues are issues with the RFC per se and should not be addressed in RFC compliant implementations. I think our implementation is fine for now

lib/std/Uri.zig Outdated
const std = @import("std.zig");
const testing = std.testing;

scheme: ?[]const u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

scheme is not optional, all forms of URI require a scheme (just like they require a path)

Copy link
Member

Choose a reason for hiding this comment

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

what about host?

Copy link
Contributor

@truemedian truemedian Jan 7, 2023

Choose a reason for hiding this comment

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

URI encompass URNs, which have no authority. The most simple URI is scheme:something where something is the "path"

https://www.rfc-editor.org/rfc/rfc3986.html#section-3

The following are two example URIs and their component parts:

    foo://example.com:8042/over/there?name=ferret#nose
    \_/   \______________/\_________/ \_________/ \__/
     |           |            |            |        |
  scheme     authority       path        query   fragment
     |   _____________________|__
    / \ /                        \
    urn:example:animal:ferret:nose

@andrewrk andrewrk enabled auto-merge January 7, 2023 01:07
@andrewrk andrewrk merged commit 87b2234 into ziglang:master Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants