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.

Sign up for GitHub

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

Jump to bottom

Fix completions within method call expr args #32796

Merged
merged 5 commits into from Sep 22, 2021
Merged

Fix completions within method call expr args #32796

merged 5 commits into from Sep 22, 2021

Conversation

nadeeshaan
Copy link
Contributor

@nadeeshaan nadeeshaan commented Sep 17, 2021 •
edited

Purpose

Fixes #19612
Fixes #32825
Fixes #21540

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@codecov
Copy link

codecov bot commented Sep 17, 2021 •
edited

Codecov Report

Merging #32796 (ab0b86e) into master (ce7bd76) will decrease coverage by 0.15%.
The diff coverage is 75.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32796      +/-   ##
============================================
- Coverage     71.55%   71.40%   -0.16%     
+ Complexity    41269    41212      -57     
============================================
  Files          2978     2979       +1     
  Lines        166481   166564      +83     
  Branches      21203    21215      +12     
============================================
- Hits         119129   118934     -195     
- Misses        40487    40791     +304     
+ Partials       6865     6839      -26
Impacted Files Coverage Δ
...ers/context/FunctionCallExpressionNodeContext.java 91.66% <ø> (ø)
...alang/langserver/diagnostic/DiagnosticsHelper.java 69.89% <ø> (ø)
...erver/contexts/AbstractDocumentServiceContext.java 70.65% <33.33%> (-1.58%) ⬇️
...ngserver/completions/util/ContextTypeResolver.java 73.40% <62.06%> (-1.39%) ⬇️
...lerinalang/langserver/common/utils/CommonUtil.java 76.25% <72.72%> (+0.24%) ⬆️
...ers/context/RemoteMethodCallActionNodeContext.java 75.00% <78.94%> (+3.00%) ⬆️
...iders/context/MethodCallExpressionNodeContext.java 86.84% <86.20%> (+5.02%) ⬆️
...ers/context/XMLNamePatternChainingNodeContext.java 100.00% <100.00%> (ø)
...ompletions/util/FieldAccessCompletionResolver.java 70.89% <100.00%> (+0.44%) ⬆️
...inalang/testerina/core/AssertionDiffEvaluator.java 0.00% <0.00%> (-84.13%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ab5d71...ab0b86e. Read the comment docs.

@nadeeshaan nadeeshaan added the Team/LanguageServer Language Server Implementation related issues. #Compiler label Sep 21, 2021
malinthar
malinthar previously approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@malinthar malinthar left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes Look Good! Shall we ensure that we have addressed the Remote Method Call Action Node as well.

mohanvive
mohanvive requested changes Sep 21, 2021
View reviewed changes
...langserver-core/src/main/java/org/ballerinalang/langserver/BallerinaTextDocumentService.java Outdated
@@ -143,7 +143,7 @@ void setClientCapabilities(LSClientCapabilities clientCapabilities) {
position.getPosition());
try {
return LangExtensionDelegator.instance()
.completion(position, context, this.serverContext, cancelChecker);
.completion(position, context, this.serverContext, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not required, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should find some other option to handle this case when debugging. There is a high possibility this temp change get committed. Worst case, a test case maybe :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mohanvive as @IMS94 suggested, let's add a system property. I'm working on the fix and we can merge it along with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #32825 to address that. May be we can close it with this PR.

IMS94
IMS94 previously approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@IMS94 IMS94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nadeeshaan nadeeshaan dismissed stale reviews from IMS94 and malinthar via 81b156d September 21, 2021 08:49
@nadeeshaan nadeeshaan merged commit 704ddcc into ballerina-platform:master Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohanvive mohanvive mohanvive approved these changes

@IMS94 IMS94 IMS94 left review comments

@malinthar malinthar malinthar left review comments

@TharushiJay TharushiJay Awaiting requested review from TharushiJay

@gayalkuruppu gayalkuruppu Awaiting requested review from gayalkuruppu

Assignees
No one assigned
Labels
Team/LanguageServer Language Server Implementation related issues. #Compiler
Projects
None yet
Milestone
No milestone
4 participants
@nadeeshaan @mohanvive @IMS94 @malinthar