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

Mark parameter to MiniSearch.removeAll as optional #70

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

nilclass
Copy link
Contributor

According to the documentation, documents is optional. However, with current typing it was not possible to call it without an argument.

I swapped the condition branches, because checking arguments.length == 0 did not rule out documents being null/undefined, so documents needs to be checked explicitly, and if (documents) seems clearer to me here than if (!documents).

According to the documentation, `documents` is optional. However, with
current typing it was not possible to call it without an argument.
@lucaong
Copy link
Owner

lucaong commented Oct 22, 2020

Hi @nilclass , great catch, thanks!

The condition has one problem now though: since an empty array is falsey in JavaScript, removeAll([]) would now remove all documents, instead of none (edit: empty array is not falsey, but the following point still stands). That is why before the check was considering the number of arguments passed: if you pass anything, that is taken to be the list of documents to remove. If one calls removeAll(x) and x happens to be falsey, there is a high chance it's unintentional, and it's better to raise an error. In order to remove all documents, one has to call removeAll() with no arguments, so it cannot happen by mistake.

One option would be to say if (documents == null), but I think that removeAll(null) should rather raise an error, as it's not a valid usage of this method. I am leaning for keeping the check on argument length. Otherwise, if (documents === undefined) would also be an option (calling removeAll(undefined) has a low chance of being unintentional).

@lucaong
Copy link
Owner

lucaong commented Oct 22, 2020

Thanks, will merge now 👍

@lucaong lucaong merged commit f8ce428 into lucaong:master Oct 22, 2020
@nilclass nilclass deleted the fix-removeall-documents-type branch October 22, 2020 15:08
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

2 participants