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

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 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

qti3e
qti3e reviewed Jun 3, 2018
View reviewed changes
Copy link
Contributor

@qti3e qti3e left a comment •
edited
Loading

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?

qti3e
qti3e reviewed Jun 3, 2018
View reviewed changes
testdata/013_console_assert.ts.out Outdated
@@ -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 •
edited
Loading

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.

qti3e
qti3e approved these changes Jun 3, 2018
View reviewed changes
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.

@kevinkassimo kevinkassimo mentioned this pull request Jun 5, 2018
Closed
@ry ry merged commit 874db2a into denoland:master Jun 5, 2018
@ry
Copy link
Member

ry commented Jun 5, 2018

@g-plane Thanks!

@justjavac justjavac mentioned this pull request Dec 13, 2018
Merged
hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024
@mmastrac
feat(core): Add resource_handle (prereq for fast streams work) (denol…
2f37bbc 
…and#102)

* feat(core): Add resource_handle (prereq for fast streams work)

* fmt

* ResourceHandleFd allows for removal of cfg

* Add missing exports, re-try windows

* reattempt windows

* Add get_fd/get_handle/get_socket to ResourceTable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@qti3e qti3e qti3e approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@g-plane @CLAassistant @yorkie @ry @qti3e @trivikr