Skip to content
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

Merged
merged 9 commits into from
Apr 4, 2023

Conversation

mkslanc
Copy link
Contributor

@mkslanc mkslanc commented Nov 25, 2022

Issue #, if available: #5004

Description of changes:

  • Fix wrong declarations for Completion and Completer
  • Add description for Completer structure
  • Add two internal properties range to support range replacements and command for different kind of commands after insertion of completion (for now it's start new autocomplete)
  • Add ability to use separate getDocTooltip for any completer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Patch coverage: 94.11% and project coverage change: +0.16 🎉

Comparison is base (3c149a9) 86.47% compared to head (49ba187) 86.63%.

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     
Flag Coverage Δ
unittests 86.63% <94.11%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ext/language_tools.js 68.69% <25.00%> (-2.74%) ⬇️
src/snippets.js 87.11% <85.71%> (+0.03%) ⬆️
src/autocomplete.js 78.44% <90.47%> (+11.18%) ⬆️
src/autocomplete_test.js 99.05% <100.00%> (+1.55%) ⬆️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkslanc mkslanc marked this pull request as ready for review November 28, 2022 11:51
ace.d.ts Outdated Show resolved Hide resolved
@dodgex
Copy link
Contributor

dodgex commented Nov 29, 2022

Thank you for creating this PR.

I added some comments with thoughts on a few of the changes.

@@ -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);
Copy link
Member

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?

Copy link
Contributor Author

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)

src/ext/language_tools.js Outdated Show resolved Hide resolved
@mkslanc mkslanc requested a review from nightwing March 9, 2023 16:17
Copy link
Contributor

@andredcoliveira andredcoliveira left a 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.

ace.d.ts Outdated Show resolved Hide resolved
src/autocomplete.js Outdated Show resolved Hide resolved
@andredcoliveira andredcoliveira dismissed their stale review March 15, 2023 15:44

Checking another potentially conflicting PR first

@InspiredGuy
Copy link
Contributor

hey @mkslanc could you please fix the merge conflicts?

@mkslanc
Copy link
Contributor Author

mkslanc commented Mar 30, 2023

hey @mkslanc could you please fix the merge conflicts?

Hey @InspiredGuy , sure, done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants