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

skip empty event #55

Merged
merged 5 commits into from
Oct 11, 2023
Merged

Conversation

sirenkovladd
Copy link
Member

Fixes: #54

@sirenkovladd
Copy link
Member Author

throw new Error('should not be called');
});

bench.remove('non-existent');
Copy link
Contributor

Choose a reason for hiding this comment

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

check how vitest tests for non throwing.

Copy link
Member Author

Choose a reason for hiding this comment

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

No errors, just an empty event received in the listener

Copy link
Member

Choose a reason for hiding this comment

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

There is expect.unreachable() for situations like this

Copy link
Member Author

Choose a reason for hiding this comment

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

Approval required to update version for "vitest"

Copy link
Contributor

@Uzlopak Uzlopak Oct 10, 2023

Choose a reason for hiding this comment

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

Update to latest vitest if necessary

test/index.test.ts Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 13, 2023

The question, which has to be discussed before hand is, if it makes sense to ignore the non existing task, or to throw a custom error.

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
@sirenkovladd
Copy link
Member Author

I don't have much respect for throwing errors
I suggest returning boolean instead of this

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 13, 2023

with returning this we allow chaining/ train wreck pattern.

@sirenkovladd
Copy link
Member Author

are you sure adding remove to the chain is a good idea?

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Aslemammad Aslemammad merged commit 9a53a51 into tinylibs:main Oct 11, 2023
3 checks passed
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.

Got error when remove non-exist task
4 participants