-
-
Notifications
You must be signed in to change notification settings - Fork 815
feat: add fuzzy completions extension in the REPL #2493
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
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.
@Snehil-Shah Finally got around to taking a closer look. Not seeing any major issues and looks good overall.
There is a bit of code duplication in the various complete functions. Do you see any way of abstracting away some of it into a reusable function?
Co-authored-by: Philipp Burckhardt <[email protected]> Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
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.
@Planeshifter Do you mean a file like this w.r.t code duplication?
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.
@Snehil-Shah Yes, was wondering whether we can consolidate some of it. Your call!
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.
Ok, but I'm kinda confused with how we can abstract those😅. Do you have something in mind?
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.
Couldn't we define a helper function like
function collectCompletions( items, filter, isFuzzy ) {
var fuzzyResults = [];
var out = [];
var match;
var item;
var i;
for ( i = 0; i < items.length; i++ ) {
item = items[ i ];
if ( startsWith( item, filter ) ) {
out.push( item );
}
}
out.sort();
// Only perform fuzzy search if no exact matches are found...
if ( !isFuzzy || out.length !== 0 ) {
return out;
}
for ( i = 0; i < items.length; i++ ) {
item = items[ i ];
match = fuzzyMatch( item, filter );
if ( match ) {
fuzzyResults.push( match );
}
}
fuzzyResults = sortFuzzyCompletions( fuzzyResults );
for ( i = 0; i < fuzzyResults.length; i++ ) {
out.push( fuzzyResults[ i ] );
}
return out;
}
and then use them in the various completers? E.g., for complete_settings.js
, could directly invoke
var completions = collectCompletions( SETTINGS_NAMES, value, isFuzzy );
and then push the completions to the out
array. I realize that some of the completers have additional logic like the file system one, but may be worth exploring...
Resolves #1845
Description
This pull request:
Related Issues
This pull request:
Questions
No.
Other
fuzzy.mp4
Checklist
@stdlib-js/reviewers