Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tests for samples #453

Merged
merged 2 commits into from
Dec 6, 2017
Merged

Tests for samples #453

merged 2 commits into from
Dec 6, 2017

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Dec 1, 2017

@chalin chalin added the test.general Relates to unit, integration, perf testing label Dec 1, 2017
@chalin chalin added this to the 1.24 milestone Dec 1, 2017
@chalin chalin requested a review from filiph December 1, 2017 16:40
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Dec 1, 2017
chalin
chalin commented Dec 1, 2017
View reviewed changes
src/_samples/index.md
{% prettify dart %}
// Importing core libraries
import 'dart:async';
import 'dart:math';

// Importing libraries from external packages
import 'package:angular2/angular2.dart';
import 'package:test/test.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that external package would do and so went with test. Avoiding angular here means that we avoid the delays on precompiling it when pub get/upgrade is run.

chalin
chalin commented Dec 1, 2017
View reviewed changes
src/_samples/index.md Outdated
class Probe extends Spacecraft with Describable {
Probe(String name) : super(name, new DateTime.now());
}
{% endprettify %}
Copy link
Contributor Author

@chalin chalin Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I've added example of the use of the abstract class, though as a mixin. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. @filiph what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this alone helps explain how this works. If we're adding sample code, I'd suggest adding:

var probe = new Probe("probe");  // TODO(chalin): some better name
probe.describeWithEmphasis();  // Method taken from the Describable class.

Also not sure whether introducing a mixin in the example is a good idea. It might be confusing that we're suddenly showing a mixin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then I'll just pull that out.

@chalin chalin mentioned this pull request Dec 1, 2017
Closed
20 tasks
@chalin chalin force-pushed the chalin-samples-1201 branch 2 times, most recently from 17b2a7f to 48a1789 Compare December 5, 2017 19:58
@chalin chalin requested a review from kwalrath December 5, 2017 21:12
kwalrath
kwalrath reviewed Dec 5, 2017
View reviewed changes
src/_samples/index.md Outdated
{% prettify dart %}
class Manned {
int astronauts;
class Piloted {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice change!

src/_samples/index.md Outdated
{% prettify dart %}
class MockSpaceship implements Spacecraft {
// ...
// ···
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the changes from ... to ···. It looks strange to me because of the vertical alignment—like you're showing single-quote characters, rather than an ellipsis (which I expect to look like [or be] three periods).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, you are right. Ok, I'll have to make a sweeping change of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted to using dots in the samples code. I'll address other instances separately.

src/_samples/index.md Outdated
@@ -252,26 +268,37 @@ abstract class Describable {
}
{% endprettify %}

Any class extending `Describable` has the `describeWithEmphasis()` method, which calls the extender's implementation of `describe()`.
Any class extending `Describable` or using it as a mixin has the `describeWithEmphasis()` method, which calls the extender's / base class implementation of `describe()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

extender's / base class
->
extender or base class

(I usually avoid "/" in text, since it can be ambiguous. https://developers.google.com/style/slashes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Since the mixin has been dropped, I've reverted to the original sentence.)

src/_samples/index.md Outdated
class Probe extends Spacecraft with Describable {
Probe(String name) : super(name, new DateTime.now());
}
{% endprettify %}
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. @filiph what do you think?

src/_samples/index.md Outdated
const aSecond = const Duration(seconds: 1);
// ···
Future<Null> printWithDelay(String message) [!async!] {
await new Future.delayed(aSecond);
print(message);
}
{% endprettify %}

The code above is equivalent to:
Copy link
Contributor

Choose a reason for hiding this comment

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

code -> method

(because you added the code for const aSecond)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

src/_samples/index.md Outdated
{% prettify dart %}
Future<Null> printWithDelay(String message) async {
await new Future.delayed(const Duration(seconds: 1));
const aSecond = const Duration(seconds: 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I keep reading "aSecond" as "a second [something]", so maybe:

aSecond -> oneSecond

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@chalin
Tests for samples
614dadb 
Contributes to #407
@chalin
Copy link
Contributor Author

chalin commented Dec 6, 2017

Post-review edits done. Kathy gave a 👍 to merging, so here goes ...

@chalin chalin merged commit 74e7096 into master Dec 6, 2017
@chalin chalin deleted the chalin-samples-1201 branch December 6, 2017 03:46
@chalin
Copy link
Contributor Author

chalin commented Dec 6, 2017

@kwalrath - btw, the samples included here are from the main Samples page. They are, in a sense, a "mini sample tour" with links to larger samples. That is why I put the code for these mini samples under the examples/tours folder. Larger samples will be under examples, but under their own packages.

@chalin chalin mentioned this pull request Dec 12, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwalrath kwalrath kwalrath left review comments

@filiph filiph Awaiting requested review from filiph

Assignees
No one assigned
Labels
cla: yes Contributor has signed the Contributor License Agreement test.general Relates to unit, integration, perf testing
Projects
None yet
Milestone
1.x
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chalin @filiph @kwalrath @googlebot