AmokHuginnsson / replxx

A readline and libedit replacement that supports UTF-8, syntax highlighting, hints and Windows and is BSD licensed.
Other
690 stars 108 forks source link

Case insensitive history search and completion. #126

Closed amosbird closed 3 years ago

amosbird commented 3 years ago

After switching clickhouse-client to use the upstream replxx, we found three missing PRs in our fork which are lost.

  1. https://github.com/ClickHouse-Extras/replxx/pull/3
  2. https://github.com/ClickHouse-Extras/replxx/pull/4
  3. https://github.com/ClickHouse-Extras/replxx/pull/15

I wonder if it's ok to land)

AmokHuginnsson commented 3 years ago

I would like to have those features in separate commits and with ability to choose the feature behavior via configuration setting, e.g.: Replxx::set_ignore_case_search().

I do not want to change the current default behavior regarding "ignored case search".

Regarding "scratch", I like this feature and I even considered pulling it from Clickhouse. So definitely I want that one merged. Though I need to think about how to do it so that both behaviors are available at the same time. By both I mean: having scratch and not loosing original history line.

amosbird commented 3 years ago

I would like to have those features in separate commits and with ability to choose the feature behavior via configuration setting, e.g.: Replxx::set_ignore_case_search().

Done.

I do not want to change the current default behavior regarding "ignored case search".

Yes. It should be configurable.

By both I mean: having scratch and not loosing original history line.

That would be great! I always want a way to restore the history line back to what it is when using readline based repl. Maybe a hotkey to yank the original line back?

AmokHuginnsson commented 3 years ago

Hello. Thank you for this contribution. There are multiple issues with the provided patch though. I do not have time right now to fix problematic parts, not to mention adding regression tests for added functionality.

I have never used github for reviews of pull requests, but if that is something that would interest you we could try it.

amosbird commented 3 years ago

I do not have time right now to fix problematic parts, not to mention adding regression tests for added functionality.

That's totally understandable. I'm not familar with the codebase and the style. And writing tests also requires some preknowledge). But I'd like to try it. It should be fun.

I have never used github for reviews of pull requests, but if that is something that would interest you we could try it.

Interesting. What other reviewing technology do you prefer? I might need some help refactoring the code.

AmokHuginnsson commented 3 years ago

That is great! I think it would be easier to manage if you would create two separate pull requests. Regarding comments I could put them here in pull request code itself.

One thing that should be changed is CMakeLists.txt. Once you introduced std::optional<> you should make sure that proper version of C++ is used for building. Namely add -std=c++17 in aforementioned file.

Rest of the comments I will put in your pull requests directly.

amosbird commented 3 years ago

I think it would be easier to manage if you would create two separate pull requests.

Indeed. I've make this PR to only address case insensitive search/completion.

One thing that should be changed is CMakeLists.txt. Once you introduced std::optional<> you should make sure that proper version of C++ is used for building. Namely add -std=c++17 in aforementioned file.

I've removed the dependency of std::optional

I hope I've addressed everything. Now the only missing part would be regression tests.

BTW, what do you think of maintaining a clang-format file? Currently I have to carefully tune the added code to mimic related style and it's not easy to make it right)

amosbird commented 3 years ago

@AmokHuginnsson Friendly ping in case you missed the update)

AmokHuginnsson commented 3 years ago

I had to figure out what should be done about the semantics we discussed (choosing candidate in completions). I think I got it and I should be able to amend your pull request pretty soon.

AmokHuginnsson commented 3 years ago

I have added tests, cleaned some formatting and added semantics you mentioned (disable ignore case if pattern contains uppercase characters).

amosbird commented 3 years ago

Fantastic!