-
Notifications
You must be signed in to change notification settings - Fork 700
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
Analyze/test cmdline tutorial code excerpts #1902
Conversation
@kwalrath: I've tried to stage my changes but I can't seem to get that to work (even if I re-login, and update to the latest firebase-tools). Edit: for the record, I had to logout before re-logging in; then deployment worked once again. |
10bb19a
to
28429a3
Compare
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.
Notes for @kwalrath.
28bbe61
to
eae6375
Compare
Staged at https://dart-dev-staging-0.firebaseapp.com/tutorials/server/cmdline (link added to opening comment). |
0b37f10
to
9bf8417
Compare
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 like most of your changes very much. Most of my comments are of the nitpicky variety, and some of them don't involve your changes. But since we're in there...
General question: should references to |
Updates done and staging server refreshed. I've added a few questions as comments. (External links rechecked ✔️) |
7416751
to
e63448c
Compare
e63448c
to
d1b4415
Compare
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!
We should probably have a DPE check the code... :/ |
I'm inclined to avoid using backticks for them, although I suspect backticks have been creeping in. Here's my thinking:
But I'm not convinced I'm right. :) What do you think, @chalin? |
No it's because when I build for a PR, I used a fixed "site generation" date. That makes it easier to do full-generated-site file diffs (which I do regularly). Here is the command line I use: $ ./tool/serve.sh --pin-now --dev |
Let's go with your preference for now. Thanks for the feedback. |
ed4c0c6
to
22c74d1
Compare
@johnpryan - do you think that you'll be able review this today? It should be quick to review. The |
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.
Looks good to me, just a minor nitpick.
Resolves first item of #1803. (Builds on #1900, #1901.)
Staged at https://dart-dev-staging-0.firebaseapp.com/tutorials/server/cmdline
Original https://dart.dev/tutorials/server/cmdline
dcat.dart
example, which comes from https://github.com/dart-lang/dart-tutorials-samples/tree/master/cmdline/bin (we should consider removing it from that repo).Note:
dcat
, and the rest of the newly added code, is analyzed but NOT tested; we don't test other command line apps in this repo -- doing so would require some refactoring or additional recripting, which probably isn't worth it at this point.I've ensured that all external links in the page are valid: