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

Throw a TypeError instead of a AssertionError #1199

Merged
merged 2 commits into from
Jun 6, 2018
Merged

Throw a TypeError instead of a AssertionError #1199

merged 2 commits into from
Jun 6, 2018

Conversation

initial-wu
Copy link
Contributor

@initial-wu initial-wu commented May 30, 2018 •
edited

Cuz AssertionError is not reading-friendly, you could not directly get where the error is emitted.
image
As TypeError seems better.
image

@codecov
Copy link

codecov bot commented May 30, 2018 •
edited

Codecov Report

Merging #1199 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1199   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files           5        5           
  Lines         374      374           
=======================================
  Hits          373      373           
  Misses          1        1
Impacted Files Coverage Δ
lib/application.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef33a79...5520863. Read the comment docs.

@initial-wu initial-wu closed this May 30, 2018
@initial-wu initial-wu reopened this May 30, 2018
@doug-wade
Copy link
Contributor

Seems like an instanceof check should throw a TypeError to me.

@dead-horse
Copy link
Member

please fix the test case

@initial-wu
Copy link
Contributor Author

l'll fix it😄

@initial-wu
Copy link
Contributor Author

@dead-horse the test case has been fixed

@dead-horse dead-horse merged commit ee1a933 into koajs:master Jun 6, 2018
@initial-wu initial-wu mentioned this pull request Jun 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dead-horse dead-horse dead-horse 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.

None yet
3 participants
@initial-wu @doug-wade @dead-horse