-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat(shell-api): Support boolean "true" with currentOp MONGOSH-1131 #1219
Conversation
packages/shell-api/src/database.ts
Outdated
if (opts === true) { | ||
opts = { $all: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want something like
if (opts === true) { | |
opts = { $all: true }; | |
if (typeof opts === 'boolean') { | |
opts = { $all: opts }; |
even though it probably won’t be used with false
as an argument much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By documentation it seems that only true
is an accepted value. It's not clear what happens using false
as value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, passing false
is the same as passing no argument, but the docs do explicitly say “Can pass either a boolean or a document”.
(The legacy shell just ignores the argument entirely if it is false: https://github.com/mongodb/mongo/blob/8c85963c5e8e6cec0ba996de80e3cc7aa5a6b39d/src/mongo/shell/db.js#L835)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense. Added this case and related test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have forgotten to git push
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly, now the ocde is updated
No description provided.