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

Add default serializer for SelectionModifier in FindNext and FindPrevious #741

Merged
merged 3 commits into from
Jul 11, 2018

Conversation

scholtzan
Copy link
Member

This is a followup PR for #704. I am not sure if it would make sense to have default serializers for wrap_around and allow_same too?
Currently, serde does not support default literals such as false (serde-rs/serde#368) so it would be necessary to write some workaround with functions.

@scholtzan scholtzan mentioned this pull request Jul 10, 2018
20 tasks
@cmyr
Copy link
Member

cmyr commented Jul 10, 2018

bool implements Default, so you should be able to just use #[serde(default)], see here.

(maybe I'm misunderstanding the problem?)

@scholtzan
Copy link
Member Author

Right! I changed it now for the bool options.

@cmyr
Copy link
Member

cmyr commented Jul 11, 2018

@scholtzan this needs a rebase as well.

@scholtzan
Copy link
Member Author

@cmyr Done

self.find_next(text, false, wrap_around.unwrap_or(false),
allow_same.unwrap_or(false),
&modify_selection.unwrap_or(SelectionModifier::Set)),
self.find_next(text, false, wrap_around, allow_same, &modify_selection),
Copy link
Member

Choose a reason for hiding this comment

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

Ah I actually have one more nit while we're in here; by convention, we preface the names of functions that are directly handling RPCs with do_, so this should be do_find_next, and below should be do_selection_for_find, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@cmyr cmyr merged commit da5396d into xi-editor:master Jul 11, 2018
@cmyr
Copy link
Member

cmyr commented Jul 11, 2018

Great, thanks for the cleanup!

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.

3 participants