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

fix: config option command_key not working #1185

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

ThomasFrans
Copy link
Contributor

No description provided.

@ThomasFrans
Copy link
Contributor Author

I had made a mistake when pattern matching the event in the layout in #1172. This is a quick fix for that. I should have double checked this.

I find it a bit weird how the order of the match matters. Maybe it would be better to not match on Event::Char(character) and instead on Event::Char(character) if self.cmdline_focus && (character == '/' || character == self.configuration.values().command_key.unwrap_or(':')). It's way longer but it would prevent the order of items in the match expression from causing confusing behavior when the next person is working on it.

I don't know what to choose between smaller match patterns for readability (the way it is in this PR) or more precise ones. Maybe there are general guidelines for which one to prefer in the Rust documentation somewhere, but I'm not aware of any.

@hrkfdn
Copy link
Owner

hrkfdn commented Jun 5, 2023

I find it a bit weird how the order of the match matters. Maybe it would be better to not match on Event::Char(character) and instead on Event::Char(character) if self.cmdline_focus && (character == '/' || character == self.configuration.values().command_key.unwrap_or(':')). It's way longer but it would prevent the order of items in the match expression from causing confusing behavior when the next person is working on it.

Agreed. I'd say that if changing the order of match conditions changes behavior that's too implicit and would get lost over time.

@hrkfdn hrkfdn merged commit 6c990b5 into hrkfdn:main Jun 6, 2023
@hrkfdn
Copy link
Owner

hrkfdn commented Jun 6, 2023

Merged, thanks for adjusting!

@ThomasFrans ThomasFrans deleted the fix-layout-shortcut branch June 6, 2023 19:09
@ThomasFrans
Copy link
Contributor Author

I hope that was the only oversight. If there are more bugs/regressions because of the refactor, I'll happily fix them 🙂

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