-
Notifications
You must be signed in to change notification settings - Fork 3
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
257 autocomplete options in flowr repl #630
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 71.49% 71.10% -0.39%
==========================================
Files 217 217
Lines 6984 7046 +62
Branches 1084 1097 +13
==========================================
+ Hits 4993 5010 +17
- Misses 1704 1749 +45
Partials 287 287 ☔ View full report in Codecov by Sentry. |
src/cli/repl/core.ts
Outdated
|
||
const replCompleterKeywords = Array.from(commandNames, s => `:${s}`) | ||
|
||
/** | ||
* Used by the repl to provide automatic completions for a given (partial) input line | ||
*/ | ||
export function replCompleter(line: string): [string[], string] { | ||
const singleCommand = replCompleterKeywords.find(k => (k + ' ') === line) | ||
if(singleCommand !== undefined){ | ||
const applicableCommand = getCommand(singleCommand.substring(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wieso der substring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weil die gespeicherten Commands noch nicht mit dem ":" beginnen sondern direkt anfangen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wäre hier ein "singleCommand.replace(':', '')" besser für das Verständnis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ne ich würds auslagern in ein toCommand
oder so :D uuuund vlt noch nen guard ob es wirklich mit :
beginnt!
src/cli/repl/core.ts
Outdated
const singleCommand = replCompleterKeywords.find(k => (k + ' ') === line) | ||
if(singleCommand !== undefined){ | ||
const applicableCommand = getCommand(singleCommand.substring(1)) | ||
guard(applicableCommand !== undefined, 'Command should be defined') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ja, aber nur durch den find check in Zeile 24
@@ -11,13 +11,40 @@ import { commandNames, getCommand, standardReplOutput } from './commands' | |||
import * as readline from 'node:readline' | |||
import { splitAtEscapeSensitive } from '../../util/args' | |||
import { executeRShellCommand } from './commands/execute' | |||
import { guard } from '../../util/assert' | |||
import { scripts } from '../common/scripts-info' | |||
import type { OptionDefinition } from 'command-line-usage' | |||
|
|||
const replCompleterKeywords = Array.from(commandNames, s => `:${s}`) | |||
|
|||
/** | |||
* Used by the repl to provide automatic completions for a given (partial) input line | |||
*/ | |||
export function replCompleter(line: string): [string[], string] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests hierfür wären bestimmt gut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
du meinst für den replCompleter? (Unklar weil der Kommentar auch zu den "import"-Statements ist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
die sind nur dabei weil GitHub immer kontext liefert, es ist die unterste Zeile :D also ja für den completer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vielleicht gleich einen Ort spezifizieren wo sie reinkommen sollten (also datei angeben und ob die neu erstellt werden muss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/functionality/cli/repl.ts
cli und repl müssen neu erstellt werden
src/cli/repl/core.ts
Outdated
const applicableCommand = getCommand(singleCommand.substring(1)) | ||
guard(applicableCommand !== undefined, 'Command should be defined') | ||
if(applicableCommand.script){ | ||
let scriptOptions: OptionDefinition[] | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
die suche nach dem skript welches wir möchten sollten wir auslagern
src/cli/repl/core.ts
Outdated
const autoCompleteList : string[] = [] | ||
autoCompleteList.push(line) | ||
scriptOptions.forEach(option => { | ||
autoCompleteList.push(line + ' --' + option.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wir müssen noch schauen, dass wir es so hinkriegen, dass man auch mehrere Optionen angeben kann usw. Da gibt es bestimmt schon eine Bibliothek oder eine Möglichkeit das mit readline hinzubekommen. Kannst du da mal schauen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meinst du jetzt eine Permutation der Optionen? Sowas wie: Wenn es -a und -b für command gibt, dass dann in der liste [command -a, command -b , command -a -b] steht?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ich mein, dass man auch bei foo --bar --baz
noch weiter completen kann, aber dann nur die Optionen kriegt die auch mehrfach gehen. Es sollte da auch sicher pakete geben (oder: https://stackoverflow.com/questions/44708200/nodejs-readline-autocomplete-several-words)
src/cli/repl/core.ts
Outdated
if(option.alias !== undefined){ | ||
autoCompleteList.push(line + ' -' + option.alias) | ||
const splitCommandList = splitAtEscapeSensitive(line) | ||
console.log(splitCommandList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take care to remove these logging statements in the end!
const splitCommandList = splitAtEscapeSensitive(line) | ||
console.log(splitCommandList) | ||
//find command in commandlist | ||
const keyword = replCompleterKeywords.find(k => splitCommandList[0] === k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this check we are probably better off with a set, given that more and more commands are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for several keywords to be exactly equal?
In this instance it can only be one keyword. Shouldn't multiple commands never be mapped to one keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comment was about the speed - with find you do a linear search on all keywords. for the autocompletion to be faster, with more and more commands, i think a set would be fine because we only check if a given command exists.
src/cli/repl/core.ts
Outdated
//console.log(scriptOptions)//TODO REMOVE | ||
const possibleOptions : string[] = [] | ||
getPossibleCommandLineOptions(scriptOptions,splitCommandList).forEach(o => { | ||
possibleOptions.push('--' + o.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels like we should abstract away from the constant --
and -
prefix-addition.
Recreating this as a new PR & branch |
No description provided.