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 the case-(in)sensitivity related issues in the matcher #29

Closed
romix opened this issue May 26, 2016 · 5 comments
Closed

Fix the case-(in)sensitivity related issues in the matcher #29

romix opened this issue May 26, 2016 · 5 comments

Comments

@romix
Copy link

romix commented May 26, 2016

The matcher currently seems to be always lower the strings, before it does the actual matching.

This is fine in most cases. But sometimes it leads to strange results. For example, if you are in a regex mode and you look just for a regex which is a simple string "MySubstring" (not the two uppercase letters), you won't get any results, even if the actual original line contains it. This happens, because the regex is case-sensitive, but the lines it is being matched against are not, since they are lowered.

You should probably either avoid lowering lines in some cases.
Or you should use the case insensitive matching mode for regexes, which can be easily done by replacing the current:

prog = re.compile(regex)

by

prog = re.compile(regex, re.IGNORECASE)
@FelikZ
Copy link
Owner

FelikZ commented May 31, 2016

Hi @romix,

The reason why current matching is lowering everything is because it works much faster then with IGNORECASE flag.

I suggest to convert user input to lower case - in that case your example will find something.

@FelikZ FelikZ added the bug label May 31, 2016
@romix
Copy link
Author

romix commented May 31, 2016

@FelikZ OK, so you do it for performance reasons. I see.

In this case, how about lowercasing the search string/regex before you compile it? I think it should be done by your plugin just before the re.compile line. Of course you could ask the users to do so before they call your plugin, but IMHO this would be wrong, because your plugin is supposed to be a drop-in replacement for CtrlP matching functions. And CtrlP does not have a requirement to lowercase the search strings/regexes.

If you decide not to lowercase the search string/regex and expect users to do so before a call, I'd suggest to clearly document this. I.e. that different from the standard CtrlP matching function, your plugin expects all search strings/regexes to be in the lowercase to function properly.

@FelikZ
Copy link
Owner

FelikZ commented Jun 21, 2016

@romix I think this is good idea to do both - force lower case the query and document this. Again, idea behind this plugin is performance. If you know the way how to do insensitive searches with the same speed - it would be nice to share it :)

@FelikZ FelikZ added enhancement and removed bug labels Jun 21, 2016
@romix
Copy link
Author

romix commented Jun 21, 2016

@FelikZ Yes, I've run some benchmarks and re.IGNORECASE is much slower in many cases.
But another approach that I mentioned seems to work just fine, namely lowercasing the regex before you call re.compile on it. It does not lead to performance degradation and it finds proper strings.
Would do you think about adding this change to your code?

@FelikZ
Copy link
Owner

FelikZ commented Jun 22, 2016

@romix that sounds logical to me now, so I agree with that.

@FelikZ FelikZ closed this as completed in fb831ff Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants