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

Added better overview, examples, and links #52612

Closed
wants to merge 1 commit into from

Conversation

AMDphreak
Copy link

Added better overview, disambiguated examples, added internal reference links for class properties.

The examples and overview of the URI (Uri) class in its documentation were lacking some important contextual information that is direly needed for newer programmers. I did the best I could to patch it up. I'm newish to Dart. I didn't even use dart format because I didn't actually pull this all the way down to my local machine. Edited right here in Github.


  • [ x ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.


Added better overview, disambiguated examples, added internal reference links for class properties.
@copybara-service
Copy link

Thank you for your contribution. This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/307420

Please wait for a developer to review your code review at the above link. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. You can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code and they will help you. You can also push additional commits to this pull request to update the code review.

@mraleph
Copy link
Member

mraleph commented Jun 7, 2023

@AMDphreak thank you for the contribution! @lrhn have left some review comments on the Gerrit CL, please take a look when you have a chance.

@devoncarew devoncarew requested a review from lrhn June 21, 2023 21:22
@mraleph
Copy link
Member

mraleph commented Oct 19, 2023

@lrhn should this PR be closed (are you taking over?)

@lrhn
Copy link
Member

lrhn commented Oct 19, 2023

Thanks for reminding me. I'll try to get out back into noon-conflicting shape and land it.

@mraleph
Copy link
Member

mraleph commented Nov 20, 2023

@lrhn ping

@vsmenon
Copy link
Member

vsmenon commented Jan 29, 2024

gentle ping

@@ -24,24 +24,52 @@ const int _LOWER_CASE_Z = 0x7A;

const String _hexDigits = "0123456789ABCDEF";

/// A parsed URI, such as a URL.
/// A parsed URI, such as a URL. URI is Uniform Resource Identifier. URL is
/// Uniform Resource Locator, a.k.a. a web address.
Copy link
Member

Choose a reason for hiding this comment

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

First paragraph of a DartDoc should be a single one-line sentence.

I'd move the added sentences to a paragrpah of their own. Hmm, maybe with links to definitions.

Or include URI

/// A parsed Uniform Resource Identifier (URI), such as a URL.
///
/// A URL, Uniform Resource Locator, is for example a web address or
/// local file path.
/// A URL expresses *how* to access a resource. All URLs are URIs.
///
/// A [URI][rfc3986] is an resource *identifier*, which means it may uniquely define
/// something without necessarily providing a way to access that thing
/// from the URI. 
/// One example is a mail-link like `mailto:someone@example.com`, which
/// identifies a mailbox, but doesn't provide a way to access it.
/// Most URIs being used are URLs, the most common ones being URIs with the
/// `http`, `https` and `file` schemes.
///
/// [rfc3986]: https://datatracker.ietf.org/doc/html/rfc3986 "RFC 3986"

/// locations (`C:\file` on Windows or `/file` on Unixes), or local folder
/// locations (`C:\folder\` or `/folder/`).
///
/// You can create web URIs either by specifying the scheme (protocol) as a
Copy link
Member

Choose a reason for hiding this comment

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

(We generally don't address the reader in DartDocs. I'm not sure that's always the right choice, sometimes it makes the phrasing more convoluted, but it is a style that's farily consistent.)

///
/// httpsUri = Uri(
/// The optional parts of the URL at the end come in two flavors: they can
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth introducing the format of a hierarchical URI (which this class is generally representing),
before talking about the optional parts at the end.

/// A hierarchical URI has the following format:
/// ```
/// scheme://username:password@host:port/path?query#fragment
/// ```
/// where all parts are optional. 

If that precedes the "To create a URI", it explains the names of the parameters of Uri.

///
/// httpsUri = Uri(
/// The optional parts of the URL at the end come in two flavors: they can
/// be fragments or query parameters. For convenience, the query Params
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mention the fragment here. Just:

/// The _query_ part of a URI traditionally contains encoded key/value pairs, 
/// originally the values of an HTML form. For convenience, the query 
/// can be provided as `queryParameters`, a `Map` from parameter names to their values, 
/// which are either a single string or an `Iterable<String>` when the same name has
/// multiple values.
///
/// For example this code uses the `queryParameters`:
/// ```
/// ...
/// ```

///
/// Make a mailto link to email someone in the user's default Mail application:
Copy link
Member

Choose a reason for hiding this comment

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

Don't think mentioning a "default Mail application" is useful here. Maybe as an example, but that still presumes that the code runs in a setting where there is a default mail application.

/// The value is the empty string if there is no fragment identifier
/// component.
String get fragment;

/// The URI path split into its segments.
/// Same as the URI [path], but split into its segments.
Copy link
Member

Choose a reason for hiding this comment

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

So it's not the same. I'd use the original phrasing, but add the [path] link.

/// Same as the URI [path], but split into its segments.
///
/// This is an alternative way to access the [path] component and
/// is useful for when a program needs to access components of a URI
Copy link
Member

Choose a reason for hiding this comment

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

access components of a URI -> work with the segments of a URI path

/// This is an alternative way to access the [path] component and
/// is useful for when a program needs to access components of a URI
/// such as the immediate parent folder of the current file.
/// This kind of field is valuable for extracting folder names
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the "kind of field", it's not clear what it means. The folder names was mentioned jsut above.

So maybe just

  /// Where the [path] contains the URIs path component, including any escapes,
  /// the individual segments of `pathSegments` have been unescaped and can,
  /// for example, easily be displayed in a breadcrumb display.
I'd proably just drop it. I wouldn'

/// For example, if iterating through files in several local folders
/// that have a similar folder and file naming scheme, you could modify
/// `pathSegments` to hit every file that matches a pattern within every
/// folder that matches a pattern.
Copy link
Member

Choose a reason for hiding this comment

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

I think that paragraph is too vague to be useful.

Giving lots of examples of ways to use a value means attempting to guess the user's needs.
If we guess wrong, it's just noise to the reader, useless examples for something they're not doing.

Each example should show something generally useful

///
/// If the port number is the default port number
/// (zero for unrecognized schemes, with http (80) and https (443) being
/// recognized),
/// then the port is made implicit and omitted from the URI.
bool get hasPort;

/// Whether the URI has a query part.
/// Whether the URI has a [query] part.
Copy link
Member

Choose a reason for hiding this comment

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

I do like the link, but it's technically a little misleading.
The Uri always has a [query] getter. It may or may not have a query component. (If not, the query getter returns the empty string.)

Keep it for now. It links to useful information.

@lrhn
Copy link
Member

lrhn commented Jan 29, 2024

(These are all "notes to self", since I should be doing this.)

@mraleph
Copy link
Member

mraleph commented Apr 30, 2024

I assume this is stale. Please reopen if not.

@mraleph mraleph closed this Apr 30, 2024
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.

None yet

4 participants