Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
63283006:
Add a README describing how to contribute to pub. (Closed)
|
Created:
7 years, 1 month ago by nweiz Modified:
7 years, 1 month ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd a README describing how to contribute to pub.
R=rnystrom@google.com
BUG=8415
Committed: https://code.google.com/p/dart/source/detail?r=30316
Patch Set 1 #
Total comments: 22
Patch Set 2 : code review #
Total comments: 4
Patch Set 3 : code review #Messages
Total messages: 6 (0 generated)
This is awesome! I made a bunch of suggestions. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.md File sdk/lib/_internal/pub/README.md (right): https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:1: # Working on pub "Contributing to pub"? "Working" sounds like... work. :) Before we get people to run off and start coding, it might be good to suggest that they get in touch first and tell us what they're interested in working on. It would suck if they spent a bunch of time on something that we were working on too. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:4: large and can be hard to understand. This document is intended to get The "but" is a bit negative here. How about: Thanks for being interested in contributing to pub! Contributing to a new project can be hard: there's a lot of new code and practices to learn. This document... https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:19: types "pub" on the command line or runs it in the Dart editor. May also want to mention the actual shell scripts in sdk/bin here too. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:25: particular pub command. All the commands are encapsulated in files in I think this reads better singular: "Each command is encapsulated in a file in..." https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:30: To run pub from the Dart repository, first build Dart. From the root of the Link to the page for building the SDK for more details here, especially the initial setup. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:46: Most changes to pub should be accompanied by one or more tests that exercises "Most changes" -> "Changes". Exceptions are rare. :) Also "exercises" -> "exercise". https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:49: use of the [scheduled_test] package in its tests, so it's usually important to be Long line. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:56: used when each test will take a short time to run. Give an example here like version_test. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:59: commands. Each test has a file to itself, which is named after the test Mention most tests are of this form. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:61: the tests time out when running on the build bots. Give an example. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:67: ./tools/test.py -m release pub/get/relative_symlink_test How about a short section at the end for getting your patch landed? You can mostly just link to the contributor guide, but that makes sure this doc covers the full workflow.
https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.md File sdk/lib/_internal/pub/README.md (right): https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:1: # Working on pub On 2013/11/15 00:14:03, Bob Nystrom wrote: > "Contributing to pub"? "Working" sounds like... work. :) Done. > Before we get people to run off and start coding, it might be good to suggest > that they get in touch first and tell us what they're interested in working on. > It would suck if they spent a bunch of time on something that we were working on > too. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:4: large and can be hard to understand. This document is intended to get On 2013/11/15 00:14:03, Bob Nystrom wrote: > The "but" is a bit negative here. How about: > > Thanks for being interested in contributing to pub! Contributing to a new > project can be hard: there's a lot of new code and practices to learn. This > document... Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:19: types "pub" on the command line or runs it in the Dart editor. On 2013/11/15 00:14:03, Bob Nystrom wrote: > May also want to mention the actual shell scripts in sdk/bin here too. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:25: particular pub command. All the commands are encapsulated in files in On 2013/11/15 00:14:03, Bob Nystrom wrote: > I think this reads better singular: "Each command is encapsulated in a file > in..." Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:30: To run pub from the Dart repository, first build Dart. From the root of the On 2013/11/15 00:14:03, Bob Nystrom wrote: > Link to the page for building the SDK for more details here, especially the > initial setup. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:46: Most changes to pub should be accompanied by one or more tests that exercises On 2013/11/15 00:14:03, Bob Nystrom wrote: > "Most changes" -> "Changes". Exceptions are rare. :) > Also "exercises" -> "exercise". Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:49: use of the [scheduled_test] package in its tests, so it's usually important to be On 2013/11/15 00:14:03, Bob Nystrom wrote: > Long line. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:56: used when each test will take a short time to run. On 2013/11/15 00:14:03, Bob Nystrom wrote: > Give an example here like version_test. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:59: commands. Each test has a file to itself, which is named after the test On 2013/11/15 00:14:03, Bob Nystrom wrote: > Mention most tests are of this form. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:61: the tests time out when running on the build bots. On 2013/11/15 00:14:03, Bob Nystrom wrote: > Give an example. Done. https://codereview.chromium.org/63283006/diff/1/sdk/lib/_internal/pub/README.... sdk/lib/_internal/pub/README.md:67: ./tools/test.py -m release pub/get/relative_symlink_test On 2013/11/15 00:14:03, Bob Nystrom wrote: > How about a short section at the end for getting your patch landed? You can > mostly just link to the contributor guide, but that makes sure this doc covers > the full workflow. Done.
Couple of suggestions but LGTM! https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... File sdk/lib/_internal/pub/README.md (right): https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... sdk/lib/_internal/pub/README.md:5: document is intended to get contributors up and running as quickly as possible. "contributors" -> "you" https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... sdk/lib/_internal/pub/README.md:12: [rnystrom@google.com] letting us know that you're interested in contributing and Maybe just point them at misc@?
https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... File sdk/lib/_internal/pub/README.md (right): https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... sdk/lib/_internal/pub/README.md:5: document is intended to get contributors up and running as quickly as possible. On 2013/11/15 01:44:09, Bob Nystrom wrote: > "contributors" -> "you" Done. https://codereview.chromium.org/63283006/diff/60001/sdk/lib/_internal/pub/REA... sdk/lib/_internal/pub/README.md:12: [rnystrom@google.com] letting us know that you're interested in contributing and On 2013/11/15 01:44:09, Bob Nystrom wrote: > Maybe just point them at misc@? Done.
Message was sent while issue was closed.
Committed patchset #3 manually as r30316 (presubmit successful). |
