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

(Potentially) incorrect handling of trailing \ in MultiPattern::reparse #66

Open
alexrutar opened this issue Dec 9, 2024 · 1 comment · May be fixed by #67
Open

(Potentially) incorrect handling of trailing \ in MultiPattern::reparse #66

alexrutar opened this issue Dec 9, 2024 · 1 comment · May be fixed by #67

Comments

@alexrutar
Copy link
Contributor

alexrutar commented Dec 9, 2024

The docs for MultiPattern::reparse explain that the 'append' argument should be set to true if the new query string contains the old query string as a prefix.

However, it seems to me that this does not consider the case when there is a trailing \? I had to add this extra check downstream in nucleo_picker in order to get proper search results.

It seems plausible to me that this is also a bug in helix: https://github.com/helix-editor/helix/blob/db1d84256fbae21abb3ba46943fb1abb8e211355/helix-term/src/ui/picker.rs#L539

I guess either this is actually an error (and the reparse method should be fixed) or it should be clarified in the documentation that one also needs to check for the presence of a trailing \.

Edit: Seems to be a missing check here

nucleo/src/pattern.rs

Lines 53 to 59 in d17e29a

if append
&& old_status != Status::Rescore
&& self.cols[column]
.0
.atoms
.last()
.map_or(true, |last| !last.negative)

@alexrutar alexrutar changed the title Ambiguous documentation for append argument to MultiPattern::reparse (Potentially) incorrect handling of trailing \ in MultiPattern::reparse Dec 9, 2024
@pascalkuthe
Copy link
Member

pascalkuthe commented Dec 9, 2024

Yeah that is an oversight, should just include a check wether the pattern ends with a backslash. Altough I vaguely remeber I did include that but maybe forgot somewhere (or maybe I forgot to add it and just planned it)

@alexrutar alexrutar linked a pull request Dec 9, 2024 that will close this issue
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 a pull request may close this issue.

2 participants