Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Fix #92: rewrite samples using async await. #93

Merged
merged 3 commits into from
Oct 2, 2015
Merged

Fix #92: rewrite samples using async await. #93

merged 3 commits into from
Oct 2, 2015

Conversation

damondouglas
Copy link
Contributor

No description provided.

@damondouglas
Copy link
Contributor Author

Hello @kwalrath please let me know if I could make this better and thank you for considering my contribution.

@Sfshaza
Copy link
Contributor

Sfshaza commented Aug 12, 2015

Hey, Damon, I'm the other Dart writer. THANKS for doing this! I'm sure we'll want an engineer to review, but we appreciate it!
--shams

@@ -8,18 +8,19 @@

import 'dart:io';

void main() {
void main() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the "void", right?

@kwalrath
Copy link
Contributor

I'm only partway through looking at this, but thanks very much @damondouglas! I looked up your CLA status and you've signed it, so that's good.

@damondouglas
Copy link
Contributor Author

Ah I'm doing for Streams now. Thank you.

@damondouglas
Copy link
Contributor Author

@kwalrath and @Sfshaza , Thank you again for considering my contribution. I cleaned up a bit of void main's and implemented await for Streams. I confess I didn't realize this was possible with Streams. Dart is such a beautiful language.

@kwalrath
Copy link
Contributor

Great! I'll get back to reviewing this soon.

@kwalrath
Copy link
Contributor

OK, I'm back to this. While I get restarted, @damondouglas... Have you confirmed that these samples run? Have you run the formatter on them? Thanks!

.then((Directory directory) {
print(directory.path);
});
var directory = await new Directory('dir/subdir');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be .create(recursive: true) so it'll create dir, as well as subdir?

@kwalrath
Copy link
Contributor

I had a bunch of nitty little comments, but overall this looks great!

@damondouglas
Copy link
Contributor Author

Hello @kwalrath , thank you so much for your help and comments. I will address those straight away.

@damondouglas
Copy link
Contributor Author

Hello @kwalrath , Thank you for your patience! I scrubbed the code through dartfmt. Pardon, I forgot this step. I also changed some of the code according to your helpful comments.

@kwalrath
Copy link
Contributor

kwalrath commented Oct 1, 2015

Sorry, I dropped the ball on this one. Let me take another look.

printResponseBody(response);
client.close();
} catch (e) {
client.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use finally instead of duplicating client.close().

@kwalrath
Copy link
Contributor

kwalrath commented Oct 1, 2015

A couple of minor comments, but this is already so much better I'd accept it as-is if you don't have time right now to make more changes.

@damondouglas
Copy link
Contributor Author

Thank you, again. Pardon, we are moving today to Seattle so I won't be able
to look for some time.
On Oct 1, 2015 5:34 PM, "Kathy Walrath" notifications@github.com wrote:

A couple of minor comments, but this is already so much better I'd accept
it as-is if you don't have time right now to make more changes.


Reply to this email directly or view it on GitHub
#93 (comment)
.

@kwalrath
Copy link
Contributor

kwalrath commented Oct 2, 2015

Good luck on the move! I'm going to accept this PR, and open a bug reminding us to consider the remaining comments.

kwalrath added a commit that referenced this pull request Oct 2, 2015
Fix #92: rewrite samples using async await.
@kwalrath kwalrath merged commit f72851d into dart-archive:master Oct 2, 2015
kwalrath added a commit that referenced this pull request Jan 11, 2016
Fix #94: Consider 2 remaining comments in pull #93
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants