Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RUST-1588: Add RunCursorCommand #912
RUST-1588: Add RunCursorCommand #912
Changes from 12 commits
4efc673
e6626f5
dcee793
9f9737f
9e17b35
5d74bfa
69c8d51
ee5492a
960c800
6fac64b
5eeafec
7afc8a5
54185f8
0836a45
9c690a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The option type still needs to be updated here
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.
cloning shouldn't be necessary here
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.
Rather than cloning the entire options struct, it would be more efficient only to clone the
selection_criteria
. You should be able to do this by using a reference to the options, which can be done either like this:or this:
(I slightly prefer the first one for no good reason; either is totally fine.)
You can also use
and_then
to achieve the same thing:We use
as_ref
to get a reference to the struct inside theOption
rather than an owned version of it so that we can continue to use it later.and_then
does something called a flat map (you can read more about it here), which is useful when you want to avoid getting data that's wrapped in multiple layers ofOption
s/Result
s.All three of these options would work here, but it's generally considered best practice to use
and_then
rather than an explicitmatch
in situations like these.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.
Rather than creating an empty options struct when the user hasn't provided any options, we can just make the
options
field onRunCursorCommand
anOption<RunCursorCommandOptions>
. This will save us an allocation whenoptions
isNone
. (You'll probably need to do similar calls toand_then
inhandle_response
as I described above to extract fields from the options).