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

Add hover support for resource paths #41773

Merged
merged 10 commits into from
Jan 17, 2024

Conversation

mindula
Copy link
Contributor

@mindula mindula commented Nov 27, 2023

Purpose

$title

Fixes #41646, #41896

Samples

Screenshot 2023-12-11 at 14 11 14

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

@mindula mindula requested a review from mohanvive as a code owner November 27, 2023 12:01
@mindula mindula added the Team/LanguageServer Language Server Implementation related issues. #Compiler label Nov 27, 2023
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (75f1758) 76.71% compared to head (74474f6) 76.67%.
Report is 561 commits behind head on master.

Files Patch % Lines
...rinalang/langserver/hover/HoverObjectResolver.java 83.33% 2 Missing and 3 partials ⚠️
.../org/ballerinalang/langserver/hover/HoverUtil.java 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #41773      +/-   ##
============================================
- Coverage     76.71%   76.67%   -0.04%     
- Complexity    52734    53010     +276     
============================================
  Files          2878     2883       +5     
  Lines        198735   199902    +1167     
  Branches      25832    26018     +186     
============================================
+ Hits         152453   153278     +825     
- Misses        37869    38160     +291     
- Partials       8413     8464      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LakshanWeerasinghe
Copy link
Contributor

  1. How do we came up with the name Query Parameters for the function params?
  2. When we have a rest args for the query params we show the type definition as int[].... This doesn't have any meaning. I think what we need to show is int....
  3. When the param description is not documented it should show an empty string. Instead of that currently it shows a null.
  4. We can have const values as the path parameters. When we hover the action statement we can see the constant as a path parameter but the description is missing even the param is documented.
    eg: resource function get path/[A]/path1()

@mindula
Copy link
Contributor Author

mindula commented Dec 11, 2023

  1. How do we came up with the name Query Parameters for the function params?
  2. When we have a rest args for the query params we show the type definition as int[].... This doesn't have any meaning. I think what we need to show is int....
  3. When the param description is not documented it should show an empty string. Instead of that currently it shows a null.
  4. We can have const values as the path parameters. When we hover the action statement we can see the constant as a path parameter but the description is missing even the param is documented.
    eg: resource function get path/[A]/path1()

First two have been addressed with #fde9bfb
Can you provide an example where it shows null when the description is not provided? I couldn't reproduce it.
Created a new issue for the 3rd one as the goto def feature is also not working for constants used as path parameters #41861.

@nipunayf
Copy link
Contributor

nipunayf commented Dec 16, 2023

When documentation for a rest parameter is not provided (for a functional argument), it displays null instead of an empty string. Consider the following example.

client class MyClient {

    # Description.
    #
    # + param - parameter description  
    resource function accessor path(string param, int... ids) {

    }
}

public function main() {
    MyClient MyClient = new;

    MyClient->/path.accessor("test", 1, 2, 3);
}
image

This is not specific to a resource access and occurs in function call expressions, as shown in the example below. The issue is tracked with: #41896

# Description.
#
# + param - parameter description  
# + return - return value description 
function name(string param, int... ids) returns string {
    return "hello";
};

public function main() {
    _ = name("hello", 1, 2, 3);
}
image

Copy link

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@mindula mindula requested a review from nipunayf January 11, 2024 04:54
@KavinduZoysa KavinduZoysa merged commit b5a4134 into ballerina-platform:master Jan 17, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/LanguageServer Language Server Implementation related issues. #Compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Show descriptions for resource paths on hover
4 participants