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.

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

Add "base" library. Add checked mode tester. #30

Merged
merged 6 commits into from Oct 3, 2013

Conversation

davidmorgan
Copy link
Contributor

My first pull request, please take a look.

Three things for discussion:

  • The "base" library / name. A place for code that looks like or deals with language built in features, e.g. utilities for Object, the "Optional" type.
  • Where to put the checked mode check. I'm suggesting that it fits under "should be part of the language", hence putting it in base. Another idea was a new library called "testing", but I worry that such a library will accumulate code that does not belong in production code, whereas this check might be needed anywhere.
  • Testing the checked mode check. This is awkward as the test needs to know whether checks are enabled ... it looks like the editor runs the tests with checked mode enabled, so that's what the test expects. Is there any way to control this? How are the tests expected to be run?

Thanks!

import 'package:quiver/base.dart';

main() {
test('isCheckedMode', () {
Copy link
Member

Choose a reason for hiding this comment

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

Put this test in a group called 'CheckedMode', and the test itself should briefly explain what's the expected outcome, e.g. "should be true".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test since I can't figure out a way to make it make sense. I added an empty test folder for the package, not sure if that's helpful or not.

@davidmorgan davidmorgan reopened this Sep 26, 2013
Renamed base library to testing.
Moved async and time test code under testing.
@davidmorgan
Copy link
Contributor Author

I updated as suggested, please take another look:

  • Renamed library to testing and moved async+time testing under here; I took a guess at the package layout, please let me know if I was close.
  • Made the check an assert.
  • Removed the test, I can't figure out a way to make it make sense.

part of quiver.testing;

/**
* Asserts that the current runtime has checked mode enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Document type of exception thrown.

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.

@yjbanov
Copy link
Member

yjbanov commented Sep 27, 2013

I like where you are going with this!

void assertCheckedMode() {
if (_isCheckedMode == null) _isCheckedMode = _checkForCheckedMode();

assert (_isCheckedMode);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't actually work - assert()s aren't run in production mode, so this won't fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point. Changed to throw StateError.

This time I tried it by adding a "main" and verifying with dart and "dart -c". I still can't think of a way to actually test in a test, unfortunately.

@yjbanov
Copy link
Member

yjbanov commented Sep 30, 2013

I think you forgot to git push :)

@davidmorgan
Copy link
Contributor Author

So I did. Done.

On Mon, Sep 30, 2013 at 5:48 PM, Yegor notifications@github.com wrote:

I think you forgot to git push :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-25375012
.

@yjbanov
Copy link
Member

yjbanov commented Oct 2, 2013

LGTM, if LG to Justin.

/**
* Asserts that the current runtime has checked mode enabled.
*
* Otherwise, throws [StateError}.
Copy link
Contributor

Choose a reason for hiding this comment

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

closing } should be a ]

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.

@justinfagnani
Copy link
Contributor

LGTM too. Thanks!

@davidmorgan
Copy link
Contributor Author

Addressed the last comments, added the top level 'runtime.dart' file for the testing.runtime library.

How does this github thing work for "submitting" changes?

@yjbanov
Copy link
Member

yjbanov commented Oct 3, 2013

As soon as you git push your latest changes one of us will hit the "Merge pull request" button :)

@davidmorgan
Copy link
Contributor Author

Very subtle way of pointing out that I forgot to push again, thanks ;) ... done.

yjbanov added a commit that referenced this pull request Oct 3, 2013
Add "base" library. Add checked mode tester.
@yjbanov yjbanov merged commit 67b6196 into google:master Oct 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants