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 console.assert #102

Merged
merged 6 commits into from
Jun 5, 2018
Merged

Add console.assert #102

merged 6 commits into from
Jun 5, 2018

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Jun 2, 2018

I'm not sure that if the assertion failed an error should be thrown.

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@qti3e qti3e left a comment

Choose a reason for hiding this comment

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

Nice, thank you : )
btw, can we have tests?
(you should make a ts file inside testdata dir and save your expected output in some other files with the same name and a .out extension.
as an example: ./testdata/a.ts ./testdata/a.ts.out)

@g-plane
Copy link
Contributor Author

g-plane commented Jun 3, 2018

@qti3e Can you review again please?

@@ -0,0 +1 @@
You will see me.
Copy link
Contributor

Choose a reason for hiding this comment

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

it will be: "Assertion failed: You will see me. "

Copy link
Contributor

@qti3e qti3e Jun 3, 2018

Choose a reason for hiding this comment

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

you can do:
../deno 013_console_assert.ts > 013_console_assert.ts.out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that is my fault. I will update.

Copy link
Contributor

Choose a reason for hiding this comment

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

@g-plane No problem : )

@g-plane
Copy link
Contributor Author

g-plane commented Jun 3, 2018

CI passed.

Copy link
Contributor

@qti3e qti3e left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for addressing my concerns about the test.

@yorkie
Copy link
Contributor

yorkie commented Jun 3, 2018

https://developer.mozilla.org/en-US/docs/Web/API/console/assert Do we need follow the naming and full implementation? This patch seems not do like this.

@g-plane
Copy link
Contributor Author

g-plane commented Jun 3, 2018

@yorkie Can you give some examples?

@ry
Copy link
Member

ry commented Jun 4, 2018

Thanks! I'd rather this was tested in tests.ts. The testdata tests start up a whole process and run rather slowly.

@g-plane
Copy link
Contributor Author

g-plane commented Jun 4, 2018

@ry If we put it into tests.ts, how to check the console output?

@ry
Copy link
Member

ry commented Jun 4, 2018

@g-plane It should throw - sorry didn't catch that it didn't before - and you can use a try-catch to test it.

@g-plane
Copy link
Contributor Author

g-plane commented Jun 5, 2018

@ry Updated and CI passed.

@ry ry merged commit 874db2a into denoland:master Jun 5, 2018
@ry
Copy link
Member

ry commented Jun 5, 2018

@g-plane Thanks!

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

6 participants