Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
89433002:
Create a first-cut for a README for http package (Closed)
DescriptionCreate a first-cut for a README for http package
BUG=
R=nweiz@google.com, rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=30838
Patch Set 1 #Patch Set 2 : turn on docs for http package #
Total comments: 13
Patch Set 3 : tweaks from review #Patch Set 4 : get close to 79/80 #
Total comments: 12
Patch Set 5 : changes #Patch Set 6 : remove all mentions of installing or pub #
Messages
Total messages: 26 (0 generated)
Please take a look. Without hosted API docs, it was hard to link to API docs. There may be plenty more for us to add, but I wanted something for new users to reference.
+nweiz. I'll let him decide how G it L. https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md File pkg/http/README.md (right): https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode1 pkg/http/README.md:1: # An easy-to-use HTTP client. Have you looked at the main library comment in http.dart? You can probably just steal this all from that. https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode4 pkg/http/README.md:4: make it easy to consume HTTP resources. Nit, but can you word wrap these at 80 (or 79) columns? https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode7 pkg/http/README.md:7: server-side or command-line Dart applications. It might help to mention that this uses "dart:io". https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode22 pkg/http/README.md:22: http.read('https://www.example.com/').then((String contents) { Ditch the type annotation on the lambda, here and below. I'd probably remove the "void" from main too. Err, I'm conflicted here. I get that it helps readers know what type is passed to the lambda, but I hate to encourage people to actually annotate them. I think I'd leave it off since the API docs include the actual type signature. https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode42 pkg/http/README.md:42: Create a new `Client` to stream the bytes from a resource. You might want to explain why they are creating a client here and not above.
https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md File pkg/http/README.md (right): https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode1 pkg/http/README.md:1: # An easy-to-use HTTP client. Done. https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode4 pkg/http/README.md:4: make it easy to consume HTTP resources. Looks wrapped to me? Was it this line? https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode7 pkg/http/README.md:7: server-side or command-line Dart applications. I added a mention of dart:io https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode22 pkg/http/README.md:22: http.read('https://www.example.com/').then((String contents) { I have to respectfully disagree. Without the type annotations in the lambdas, I'm left wondering what contents is. The purpose of this sample is to be documentation. https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode42 pkg/http/README.md:42: Create a new `Client` to stream the bytes from a resource. I assume because there's no top-level convenience method to get a streaming response? Can you recommend something here that might be more insightful?
https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md File pkg/http/README.md (right): https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode4 pkg/http/README.md:4: make it easy to consume HTTP resources. On 2013/11/26 23:11:00, sethladd wrote: > Looks wrapped to me? Was it this line? I should have been clearer. :) It looks like this line and some of the later ones are wrapped at *less* than 79/80. Do you mind filling the whole line? https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode42 pkg/http/README.md:42: Create a new `Client` to stream the bytes from a resource. On 2013/11/26 23:11:00, sethladd wrote: > I assume because there's no top-level convenience method to get a streaming > response? Can you recommend something here that might be more insightful? That's a Nathan question. :)
https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md File pkg/http/README.md (right): https://codereview.chromium.org/89433002/diff/10001/pkg/http/README.md#newcode4 pkg/http/README.md:4: make it easy to consume HTTP resources. On 2013/11/26 23:18:00, Bob Nystrom wrote: > On 2013/11/26 23:11:00, sethladd wrote: > > Looks wrapped to me? Was it this line? > > I should have been clearer. :) It looks like this line and some of the later > ones are wrapped at *less* than 79/80. Do you mind filling the whole line? Done.
Bob, while we wait for Nathan, is the content otherwise LGTM? Do you see any blockers?
Nope, LGTM!
The organization of the README seems muddled to me. The first couple of examples are fine, but it doesn't provide any explanation of the more advanced features of the library, despite using some of them in the next few examples. It doesn't describe Client or explain why you'd want to use it; it doesn't explain the difference between [Request], [StreamedRequest], and the top-level functions; and it doesn't explain the composability of the package at all. I'm not sure why it isn't using something very much like the library comment in lib/http.dart. Speaking of which, if we're adding a README, we should remove that comment. There shouldn't be two places to maintain high-level overviews of the package. I also don't like that none of the classes/functions here are linked to the API docs. Even if they need to have manual links to http://test.dartdoc-next.appspot.com/#http/http, they should provide the user some way of learning more about specific functionality. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md File pkg/http/README.md (right): https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode1 pkg/http/README.md:1: # A composable, Future-based library for making HTTP requests. This shouldn't all be part of an h1. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode4 pkg/http/README.md:4: easy to consume HTTP resources. "consume HTTP resources" sounds stilted to me. Also, this doesn't just consume; it can also make modifying requests like POST and DELETE. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode6 pkg/http/README.md:6: **NOTE:** This package only works for "only works" -> "currently only works" As soon as it's feasible I plan to add support for XHRs. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode8 pkg/http/README.md:8: imports `dart:io`, it can use this package. Fix the word-wrapping in this paragraph. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode12 pkg/http/README.md:12: See the [http package on pub][hosted] for installation instructions. Since pub is widely-known now, installation instructions are no longer necessary. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode14 pkg/http/README.md:14: ## Reading text from a resource I think using REST-style "resource" terminology is going to be more confusing than it's worth. This library is designed as an HTTP client library, not a REST library. It doesn't have any notion of resources, and I think assuming the reader does will make this more confusing. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode23 pkg/http/README.md:23: http.read('https://www.example.com/').then((String contents) { Don't include type annotations for closure parameters. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode39 pkg/http/README.md:39: } I don't think this is distinct enough from the previous example to warrant being called out on its own. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode41 pkg/http/README.md:41: ## Streaming bytes from a resource Streaming isn't a common use-case; certainly not more common than other use cases that aren't covered here. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode45 pkg/http/README.md:45: import 'package:http/http.dart'; http is intended to be imported with a prefix. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode51 pkg/http/README.md:51: .then((StreamedResponse resp) { Indent this line +4 spaces. https://codereview.chromium.org/89433002/diff/40001/pkg/http/README.md#newcode87 pkg/http/README.md:87: Coming soon. The docs are now available at http://test.dartdoc-next.appspot.com/#http/http.
I've deleted all code and examples. I now just link to the docs (which, once this CL is approved, will start to appear). I believe it's OK to have a single line that tells the user how to install the package, for those users that find this README in Github, for example. thanks for taking a look.
On 2013/12/02 18:44:00, sethladd wrote: > I've deleted all code and examples. I now just link to the docs (which, once > this CL is approved, will start to appear). > > I believe it's OK to have a single line that tells the user how to install the > package, for those users that find this README in Github, for example. > > thanks for taking a look. For packages that are hosted on GitHub I can see the argument for including installation instructions, but this isn't one of those packages. Right now the primary place this README will be visible will be on pub.dartlang.org, and including a link to the page people are looking at doesn't provide any value.
I'd like to start to create a basic structure for package READMEs. I imagine developers care about: * how to install * api docs * how to file bugs * what it is * optional: an example or two I think http is really great and I'd love to see something other than stock installation instructions when I visit http://pub.dartlang.org/packages/http Is there room to compromise?
On 2013/12/02 23:56:57, sethladd wrote: > I'd like to start to create a basic structure for package READMEs. I imagine > developers care about: > > * how to install > * api docs > * how to file bugs > * what it is > * optional: an example or two > > I think http is really great and I'd love to see something other than stock > installation instructions when I visit http://pub.dartlang.org/packages/http > > Is there room to compromise? I'm like almost all of that, but it feels perverse to have a link that promises installation instructions but goes directly to the page you're already visiting. A template for READMEs is great, but there are parts of any template that don't make sense in all contexts.
I hear you, unfortunately I can't link to the installation tab directly (there's no /http#installing) Our goal is to eventually get these packages onto github. Assuming that happens, would it then make sense to include a single line about installing? I guess I'm not convinced we should assume that all developers know how to install a pub package. They might do a Google search, end up reading the README somehow, and want to know how to use it. Can you recommend some wording that helps a new developers that finds the README out of context?
On 2013/12/03 00:10:33, sethladd wrote: > I hear you, unfortunately I can't link to the installation tab directly (there's > no /http#installing) > > Our goal is to eventually get these packages onto github. Assuming that happens, > would it then make sense to include a single line about installing? I have mixed opinions about that, too -- there'll be a link at the top of the page to the pub.dartlang.org page -- but let's cross that bridge when we come to it. > I guess I'm not convinced we should assume that all developers know how to > install a pub package. They might do a Google search, end up reading the README > somehow, and want to know how to use it. > > Can you recommend some wording that helps a new developers that finds the README > out of context? I just don't think it's likely that a developer will find the README somewhere completely devoid of context. They'll either be deep in the Dart repo, at which point I think they're likely capable of fending for themself, or they'll be looking at pub.dartlang.org, where there's a big tab that says "Install".
On 2013/12/03 00:16:57, nweiz wrote: > On 2013/12/03 00:10:33, sethladd wrote: > > I hear you, unfortunately I can't link to the installation tab directly > (there's > > no /http#installing) > > > > Our goal is to eventually get these packages onto github. Assuming that > happens, > > would it then make sense to include a single line about installing? > > I have mixed opinions about that, too -- there'll be a link at the top of the > page to the http://pub.dartlang.org page -- but let's cross that bridge when we come to > it. Where do you want a link to pub.dartlang.org ? I'll add it. > > > I guess I'm not convinced we should assume that all developers know how to > > install a pub package. They might do a Google search, end up reading the > README > > somehow, and want to know how to use it. > > > > Can you recommend some wording that helps a new developers that finds the > README > > out of context? > > I just don't think it's likely that a developer will find the README somewhere > completely devoid of context. They'll either be deep in the Dart repo, at which > point I think they're likely capable of fending for themself, or they'll be > looking at http://pub.dartlang.org, where there's a big tab that says "Install". Even when the package is hosted on github?
I went to see how other packages do it. npm and express show installation instructions twice: https://npmjs.org/package/express So does async https://npmjs.org/package/async So does https://npmjs.org/package/request Those are the top-three star'ed packages on NPM. rubygems.org apparently doesn't show READMEs. I'd like to encourage packages to include how to install, so I'd like to lead by example. How about this one liner: Installation instructions at http://pub.dartlang.org/packages/http Trying to find a middle ground here. :)
On 2013/12/03 00:24:39, sethladd wrote: > On 2013/12/03 00:16:57, nweiz wrote: > > On 2013/12/03 00:10:33, sethladd wrote: > > > I hear you, unfortunately I can't link to the installation tab directly > > (there's > > > no /http#installing) > > > > > > Our goal is to eventually get these packages onto github. Assuming that > > happens, > > > would it then make sense to include a single line about installing? > > > > I have mixed opinions about that, too -- there'll be a link at the top of the > > page to the http://pub.dartlang.org page -- but let's cross that bridge when > we come to > > it. > > Where do you want a link to http://pub.dartlang.org ? I'll add it. It'll be the GitHub "website" link. It'll automatically appear at the top of the repo. > > > I guess I'm not convinced we should assume that all developers know how to > > > install a pub package. They might do a Google search, end up reading the > > README > > > somehow, and want to know how to use it. > > > > > > Can you recommend some wording that helps a new developers that finds the > > README > > > out of context? > > > > I just don't think it's likely that a developer will find the README somewhere > > completely devoid of context. They'll either be deep in the Dart repo, at > which > > point I think they're likely capable of fending for themself, or they'll be > > looking at http://pub.dartlang.org, where there's a big tab that says > "Install". > > Even when the package is hosted on github? That's still not devoid of context; the GitHub page itself will link to the pub.dartlang.org site. Also it's not on GitHub yet, so for now the only place people will see it is pub.dartlang.org.
Or, another attempt: "Find info on versions and installation instructions on pub.dartlang.org/packages/http" If we include a link back to the specific page in pub, that should be sufficient.
On 2013/12/03 00:38:16, sethladd wrote: > Or, another attempt: > > "Find info on versions and installation instructions on > pub.dartlang.org/packages/http" > > If we include a link back to the specific page in pub, that should be > sufficient. It sounds like Seth thinks installation instructions are helpful for our users. There's next to no cost to having them, and he already took the trouble to write it up. How about we just land this? I think we're probably just splitting hairs at this point and I'm sure there's more useful stuff we could be doing. :) - bob
On 2013/12/03 00:40:20, Bob Nystrom wrote: > On 2013/12/03 00:38:16, sethladd wrote: > > Or, another attempt: > > > > "Find info on versions and installation instructions on > > pub.dartlang.org/packages/http" > > > > If we include a link back to the specific page in pub, that should be > > sufficient. > > It sounds like Seth thinks installation instructions are helpful for our users. > There's next to no cost to having them, and he already took the trouble to write > it up. How about we just land this? I think we're probably just splitting hairs > at this point and I'm sure there's more useful stuff we could be doing. :) > > - bob I think it's actively frustrating for users to see a link that promises information they care about, click it, and have it take them to exactly the same page they're already on. That's the cost I'm worried about. Right now there's no benefit to having this link, and it's easy to add later if a benefit appears.
Are we certain that all users who view the README will be looking at it on pub.dartlang.org ? If so, then I agree. I'm worried about the other developers who aren't already on pub.dartlang.org.
What if I added a hash link to the installation instructions? /packages/http#installing would what help? It would be nice to have an explicit link
On 2013/12/03 00:49:48, sethladd wrote: > What if I added a hash link to the installation instructions? > > /packages/http#installing > > would what help? It would be nice to have an explicit link Currently the only place the README is surfaced is on pub.dartlang.org. Anyone else viewing it would be doing so either through the repo or in their pub cache, either of which indicate an advanced understanding of the Dart ecosystem. Linking to "/packages/http#installing" won't bring up the "Installing" tab, it'll just look like the link didn't do anything.
ptal I have removed all mentions of pub or installing
lgtm
Message was sent while issue was closed.
Committed patchset #6 manually as r30838 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
