Repository navigation
Add replacingCurrentWord completion flag and move fuzzy package to readline-api - #383
Conversation
stalep
left a comment
There was a problem hiding this comment.
Thanks for this, and sorry for the slow turnaround. Two requests before merge — one naming, one build hygiene. I'm fine with the fuzzy package move to readline-api as designed.
- Please rename
isReplaceCurrentWord(), it doesn't read naturally.
The interface convention is isX/setX with an adjective-like phrase (isIgnoreOffset,isAppendSeparator,isIgnoreStartsWith), butisReplaceCurrentWordpairs is with an imperative, so the call site reads ungrammatically:
if (!possibleCompletions.get(0).isIgnoreStartsWith()
&& !possibleCompletions.get(0).isReplaceCurrentWord()) // "is replace…?"
My preference is the present participle, which keeps the is/set symmetry and reads as plain English:
- isReplacingCurrentWord() / setReplacingCurrentWord(boolean)
Alternative, if you prefer a noun phrase in the style of isAppendSeparator: isWordReplacement() / setWordReplacement(boolean). Either works for me, just not the imperative under is.
2. Please don't hardcode the JUnit version in readline-api/pom.xml.
The parent pom already defines <junit.version>4.13.2</junit.version> (pom.xml:68) , the new test dependency should use ${junit.version}. (Yes, readline/pom.xml:91 hardcodes the same version; that's pre-existing, but if you're touching this anyway, aligning both lines would be a welcome cleanup, otherwise I'll file a follow-up.)
|
@omatheusmesmo let me know if you have any questions or disagree with my review |
|
Sorry @stalep , it's been a busy week, but I will get it done by the end of the weekend. |
Sure, no rush! |
The completion display assumed candidates extend the typed word by a prefix, which corrupts the buffer for non-prefix candidates such as fuzzy matches. The flag makes a single candidate replace the whole word, dashes included, and skips common-prefix insertion when listing. Also moves the fuzzy package to readline-api, unchanged in name, so the aesh completion fallback loads for consumers that exclude the full readline module, like jbang. * Supports aeshell/aesh#510 Signed-off-by: Matheus Oliveira <matheus.6148@gmail.com>
4b16ad6 to
618daad
Compare
|
Thanks @stalep, both points addressed and pushed:
I'll update aeshell/aesh#695 to the new method name next. |
|
Thanks a lot, merged! |
Describe Changes
The completion display assumes a candidate always extends the typed word by a shared prefix: a single candidate is appended after the prefix and multiple candidates insert their common prefix. That corrupts the buffer for completion styles where the typed word is not a prefix of the candidates, such as the fuzzy matching planned for aeshell/aesh#510: a typo like
comitwith candidatecommitappends the full name and producescomitcommit.This adds an opt-in
replacingCurrentWordflag toCompleteOperation. When set, a single candidate removes the whole typed word, dashes included, before the full candidate is written, and common-prefix insertion is skipped in favour of listing. The removal usesunix-word-ruboutbecausebackward-kill-wordtreats-as a word delimiter and leaves typed dashes behind. The flag defaults to false, so existing prefix completion behavior is unchanged.It also moves the
org.aesh.readline.fuzzypackage from thereadlinemodule toreadline-api, unchanged in name, so that the aesh completion fallback loads for consumers that exclude the fullreadlinemodule: jbang, for example, depends onaeshwithreadlineexcluded plusreadline-api. Nothing outside the package location changes; the fuzzy history search inreadlineresolves the classes through itsreadline-apidependency and all its tests keep passing.