-
Notifications
You must be signed in to change notification settings - Fork 7
Initial code commit with basic functionality, some tests, and a README #1
Conversation
Rather than a rubber stamp approval, I'd love an actual review of, specifically, a few pieces here:
|
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 great. I'm squeamish about using mirrors
but maybe I'm just being superstitious.
CONTRIBUTING.md
Outdated
|
||
All files in the project must start with the following header. | ||
|
||
// Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file |
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.
2017?
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!
lib/expected_output.dart
Outdated
} | ||
|
||
var file = p.basename(entry.path) | ||
.replaceFirst(new RegExp('\.$extension\$'), ''); |
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: this formatting looks odd. I'd expect something more like:
var file =
p.basename(entry.path).replaceFirst(new RegExp('\.$extension\$'), '');
?
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.
Or are you shying away from dartfmt
?
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.
Durp. Sorry I forgot to dartfmt 😝
library touppercase.tests; | ||
|
||
void main() { | ||
for (var dataCase in dataCasesUnder(library: #touppercase.tests)) { |
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.
What's the benefit in using reflection vs. specifying a data directory?
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.
There is no other way to get a directory relative to the executing code. (Unless there recently is.) The mirrors incantation comes from dart-lang/test#110.
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 just put a little comment in the README about this.
It's better than you think! 👍
I take this to be an implementation detail so wouldn't sweat it too much.
Same. @munificent may have some more thoughts here since it's been a while since I wrote any tests this way --- though now that I see it again, you've got me thinking! 😉
Hmmmm. As I mentioned, I'm curious about the decision to reflect rather than just specify directories. If |
@munificent, not urgent, but I'll wait a day or two in case you want to put in some input on naming. I'd rather fix it sooner than later if you hate the names or the API. |
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 nice! 👍
Thanks @pq and @munificent ! |
This is the basic functionality of expected_output. It's the same functionality in use by dart_style and markdown (and others?).