-
Notifications
You must be signed in to change notification settings - Fork 16
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
implement basic path completions #438
Conversation
d982c34
to
c41cda0
Compare
b6cadb9
to
e11d0a4
Compare
export async function listDatabases(argv) { | ||
let databases; | ||
if (argv.secret) { | ||
databases = await listDatabasesWithSecret(argv); | ||
} else { | ||
databases = await listDatabasesWithAccountAPI(argv); | ||
} | ||
return databases; | ||
} |
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.
refactored this so listDatabases is a generic API layer, and the rendering is done in doListDatabases, which is the command handler
/** | ||
* retry an assertion repeatedly until it succeeds | ||
* @param {function} evaluator - any function that throws if a condition isn't met. | ||
* @param {number} [ms=50] - the number of milliseconds to wait for the condition. set it lower than mocha's timeout to re-throw the underlying error and have usable test failure logs. | ||
*/ | ||
export async function eventually(evaluator, ms = 50) { | ||
try { | ||
return evaluator(); | ||
} catch (e) { | ||
if (ms <= 0) throw e; | ||
await new Promise((resolve) => setTimeout(resolve, 1)); // eslint-disable-line no-promise-executor-return | ||
return eventually(evaluator, ms - 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.
added this helper because deep sigh yargs' parse command resolves the promise it returns when it's done parsing... which almost always happens before it's done evaluating async completions. so we can't rely on awaiting the parse promise like we do everywhere else.
e11d0a4
to
dbf877f
Compare
.completion( | ||
"completion", | ||
async function (currentWord, argv, defaultCompletions, done) { | ||
// this is pretty hard to debug - if you need to, run | ||
// `fauna --get-yargs-completions <command> <flag> <string to match>` | ||
// for example: `fauna --get-yargs-completions --profile he` | ||
// note that you need to have empty quotes to get all matches: | ||
// `fauna --get-yargs-completions --profile ""` | ||
|
||
// then, call the done callback with an array of strings for debugging, like: | ||
// done( | ||
// [ | ||
// `currentWord: ${currentWord}, currentWordFlag: ${currentWordFlag}, argv: ${JSON.stringify(argv)}`, | ||
// ], | ||
// ); | ||
const previousWord = process.argv.slice(-2, -1)[0].replace(/-/g, ""); | ||
const currentWordFlag = Object.keys(argv) | ||
.filter((key) => previousWord === key) | ||
.pop(); | ||
|
||
// TODO: this doesn't handle aliasing, and it needs to | ||
if ( | ||
currentWord === "--profile" || | ||
currentWordFlag === "profile" || | ||
currentWord === "-p" || | ||
currentWordFlag === "p" | ||
) { | ||
done(getProfileCompletions(currentWord, argv)); | ||
} else if ( | ||
currentWord === "--database" || | ||
currentWordFlag === "database" || | ||
currentWord === "-d" || | ||
currentWordFlag === "d" | ||
) { | ||
done(await getDbCompletions(currentWord, argv)); | ||
} else { | ||
defaultCompletions(); | ||
} | ||
}, | ||
) |
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.
this is the meat of the change: yargs calls this method when zsh/bash calls fauna --get-yargs-completions ...
, and then we do some switching based on yargs argv and process.argv and call methods that either sync or async return a list of strings.
we have to use process.argv as well because yargs argv doesn't include the order of the tokens, so we can't easily find currentWordFlag
...
return regionGroups; | ||
} else { | ||
const { pageSize } = argv; | ||
buildCredentials({ ...argv, user: "default" }); |
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.
this is a glorious, if silly, hack for when we need credentials for making network calls (usually before the credentials middleware runs): just import and run the credential middleware.
dbf877f
to
874a8b0
Compare
const { pageSize } = argv; | ||
buildCredentials({ ...argv, user: "default" }); | ||
const accountClient = new FaunaAccountClient(); | ||
try { |
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.
Could we get some docstrings detailing the order of operations here for future us. Looks like we query exactly path, and one directory down if that doesn't work?
if (!getRegionGroup(currentWord)) { | ||
return regionGroups; | ||
} else { | ||
const { pageSize } = argv; |
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 probably need to debounce this code path. I pushed tab 5 times waiting for a response...which means I probably made >= 5 requests for the same exact data.
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's not really debounceable in a straightforward fashion - zsh invokes fauna
once per TAB input, fauna
runs as a process briefly, outputs the completions to stdout (which zsh reads), then exits.
since we don't have a persistent process, the only paths i see for debouncing are to customize the zsh completion script to debounce at the zsh wrapper layer, or to write/read a debounce timestamp to/from disk. both feel really icky from a maintenance, complexity, and shell portability standpoint.
i think we should have confidence in our rate limiting / service layer and just use the existing throttle protections.
874a8b0
to
93e18a0
Compare
this change adds: - the default yargs completions for commands, options, etc. - custom completions for profiles (given that you have a config specified) - custom completions for database path (given that you have specified enough information to make a request to fauna)
93e18a0
to
49083bb
Compare
this change adds: