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

Remove console.error from asserts. #483

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Remove console.error from asserts. #483

merged 1 commit into from
Jun 6, 2019

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Jun 6, 2019

I noticed the some assertions included logging to console.error. This doesn't make any sense and generally didn't include any more information than what would be contained in the AssertionError, it also causes content to get logged to console which may not be intended, like when wrapping an assertion and providing a better error message/condition to the user. We should avoid being opinionated about what gets logged to the console in std. If there are critical data (like raw "expected" and "actual" or the type of assertion made) they should be added to AssertionError so that consumers have the ability to control how the error is handled.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 2292fbe into denoland:master Jun 6, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this pull request Oct 24, 2024
Changes:
1. Notifications use ULIDs for their IDs.
2. Removes the `notifications` index from KV, as it seems unnecessary.
3. Removed the `createdAt` property from notifications.
4. Removes `newNotificationProps()`, as it now seems unnecessary.

Closes denoland#473
Towards denoland#470
CC @lino-levan (this might be a good reference to learn more about
ULIDs)
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.

2 participants