Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

search_next_impl: don't panic on invalid regex #740

Merged
merged 1 commit into from
Sep 17, 2021
Merged

search_next_impl: don't panic on invalid regex #740

merged 1 commit into from
Sep 17, 2021

Conversation

pppKin
Copy link
Contributor

@pppKin pppKin commented Sep 10, 2021

This should resolve #738

@archseer
Copy link
Member

While this fixes the issue, we probably shouldn't store searches that are invalid in the register altogether. Prompt closure would need a result value so we're able to signal errors better.

@pppKin
Copy link
Contributor Author

pppKin commented Sep 11, 2021 via email

@pppKin pppKin closed this Sep 13, 2021
@pppKin
Copy link
Contributor Author

pppKin commented Sep 13, 2021

Or perhaps we still store the invalid regex? I know vim does this(perhaps other editors too). It could be useful to use up arrow key to retrieve search histories, modify a bit and search again.

@archseer
Copy link
Member

Good point, let's merge this 👍🏻

@archseer archseer reopened this Sep 17, 2021
@pppKin
Copy link
Contributor Author

pppKin commented Sep 17, 2021

Since #651 will introduce a unified callback in regex_prompt which would make this PR a bit cleaner and tidier, let's wait till #651 is finished and merged;)

@archseer
Copy link
Member

I think this is okay to merge, the callback change won't really affect this PR (register setting was refactored on master and happens inside Prompt now)

@pppKin
Copy link
Contributor Author

pppKin commented Sep 17, 2021

Oh I see. Should be fine to merge than. If any further improvement is required we'll just open a new issue:)

@archseer archseer merged commit 1d04e59 into helix-editor:master Sep 17, 2021
@pppKin pppKin deleted the invalid_search_panic branch October 11, 2021 09:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic upon pressing n while search is set to an invalid regex
2 participants