-
Notifications
You must be signed in to change notification settings - Fork 197
Conversation
PTAL @kwalrath @legalcodes |
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.
In general looks like a big improvement.
anagram/pubspec.yaml
Outdated
build_web_compilers: ^1.0.0 | ||
pedantic: ^1.7.0 | ||
|
||
homepage: https://dart.dev/tutorials/web/low-level-html/add-elements |
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.
put this above dependencies
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.
done
cmdline/pubspec.yaml
Outdated
|
||
executables: | ||
dcat: | ||
dgrep: | ||
|
||
homepage: https://www.dartlang.org/tutorials/server/cmdline | ||
homepage: https://dart.dev/tutorials/server/cmdline |
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.
Move towards top
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.
done
anagram/pubspec.yaml
Outdated
|
||
dev_dependencies: | ||
build_runner: ^1.1.2 | ||
build_web_compilers: ^1.0.0 |
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.
Can we switch to ^2.0.0
?
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.
Yup, and as this was all based on Stagehand, I updated Stagehand's templates as well.
@@ -5,37 +5,42 @@ | |||
import 'dart:html'; | |||
import 'dart:async'; | |||
|
|||
DivElement output = querySelector('#output'); | |||
PreElement pre = PreElement(); |
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.
[nit] Don't need a type on the left.
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.
done
} | ||
|
||
// Imagine that this function is more complex and slow. :) | ||
Future gatherNewsReports() async { | ||
String path = 'https://www.dartlang.org/f/dailyNewsDigest.txt'; | ||
String path = 'https://dart.dev/f/dailyNewsDigest.txt'; |
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.
consider omitting types on the left.
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.
done
Future expensiveA() => new Future.value('from expensiveA'); | ||
Future expensiveB() => new Future.value('from expensiveB'); | ||
Future expensiveC() => new Future.value('from expensiveC'); | ||
Future expensiveA() => Future.value('from expensiveA'); |
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.
How about Future<String>
return type?
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.
done
@@ -3,6 +3,7 @@ | |||
// BSD-style license that can be found in the LICENSE file. | |||
|
|||
import 'dart:async'; | |||
import 'dart:html'; | |||
|
|||
void main() { |
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.
Should we consider using async
here?
https://dart.dev/guides/language/effective-dart/usage#prefer-asyncawait-over-using-raw-futures
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.
It is cleaner to do with async
and await
, but this directory of samples seems to be all about using Futures API.
@@ -11,15 +11,17 @@ | |||
import 'dart:async'; | |||
import 'dart:io'; | |||
import 'package:http_server/http_server.dart'; | |||
import 'package:path/path.dart'; |
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.
Typically we import this as p;
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.
done
PTAL @kwalrath, now that you are back =) |
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.
It looks like not all of these improvements are in the doc. E.g. https://dart.dev/tutorials/server/cmdline has int lineNumber = 1
.
Unfortunately, I don’t know/remember whether there’s an automatic way to update the code, especially since it’s highlighted. We might just have to eyeball it. I’ll file an issue reminding us to do this.
No description provided.