-
Notifications
You must be signed in to change notification settings - Fork 346
added explanation of Url::fragment #332
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you. :)
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file):
This should read as "the part of a url following te Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file): Previously, sigmavirus24 (Ian Cordasco) wrote…
I made a slight change to the verbiage similar to your suggestion. I thought removing the Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file): Previously, achernyak (Artem Chernyak) wrote…
Sorry, I didn't want to type the whole thing out. My suggestion is different than what you wrote and is, as a whole line,
Specifically, replace "follow" with "following". Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file): Previously, sigmavirus24 (Ian Cordasco) wrote…
This has been updated. Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file): Previously, achernyak (Artem Chernyak) wrote…
Did you push the update? It's not displaying here. Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. src/lib.rs, line 887 at r1 (raw file): Previously, sigmavirus24 (Ian Cordasco) wrote…
Sorry not sure what happened there. Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
Reviewed 1 of 1 files at r2. Comments from Reviewable |
@@ -884,6 +884,9 @@ impl Url { | |||
|
|||
/// Return this URL’s fragment identifier, if any. | |||
/// | |||
/// A fragment is the part of a url following the `#` symbol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL should be capitalized as an initialism because it appears that way in all the rest of the documentation in this crate.
@@ -884,6 +884,9 @@ impl Url { | |||
|
|||
/// Return this URL’s fragment identifier, if any. | |||
/// | |||
/// A fragment is the part of a url following the `#` symbol, | |||
/// which is used to identify a location in the document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessarily true of every URL fragment, especially if the URL does not refer to an HTML document. Could you phrase this in a more precise way? Check out this article for inspiration.
Add description of fragment to documentation Fixes #318, fixes #332. This builds on #332, and I hope addresses the comments there. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/357) <!-- Reviewable:end -->
Proposed to resolve #318.
?r @SimonSapin
This change is