-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Completions improvements #5005
Completions improvements #5005
Conversation
…letion` - `range` and `command`
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5005 +/- ##
==========================================
+ Coverage 86.47% 86.63% +0.16%
==========================================
Files 555 555
Lines 43035 43121 +86
Branches 6704 6711 +7
==========================================
+ Hits 37214 37358 +144
+ Misses 5821 5763 -58
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Thank you for creating this PR. I added some comments with thoughts on a few of the changes. |
src/ext/language_tools.js
Outdated
@@ -14,8 +14,10 @@ var keyWordCompleter = { | |||
} | |||
var state = editor.session.getState(pos.row); | |||
var completions = session.$mode.getCompletions(state, session, pos, prefix); | |||
completions = completions.map((el) => el.completerId = keyWordCompleter.id); |
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.
do we have to set completerId for all completions or can do it only when needed?
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 could define it only when needed, but I suggest to leave this for default ones (to not interfere with user defined completers)
# Conflicts: # src/autocomplete_test.js
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.
Overall looks good to me. Just a few minor type-related comments/questions.
Checking another potentially conflicting PR first
hey @mkslanc could you please fix the merge conflicts? |
# Conflicts: # src/autocomplete.js # src/snippets.js
Hey @InspiredGuy , sure, done! |
Issue #, if available: #5004
Description of changes:
Completion
andCompleter
Completer
structurerange
to support range replacements andcommand
for different kind of commands after insertion of completion (for now it's start new autocomplete)getDocTooltip
for any completerBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.