feat: Add hover info for modules and types including any docstring #212

plevold · 2022-10-30T11:37:21Z

Fixes #208

@gnikit I decided to clone the fortls repo and have a look around. Figured I actually had a shot a making #208 work myself. Here's my suggestion. Feel free to nitpick on potential issues or anything I might have overlooked!

codecov · 2022-10-30T11:39:15Z

Codecov Report

Merging #212 (60e0646) into master (abd5d34) will decrease coverage by 0.17%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
- Coverage   86.03%   85.85%   -0.18%     
==========================================
  Files          12       12              
  Lines        4439     4461      +22     
==========================================
+ Hits         3819     3830      +11     
- Misses        620      631      +11

Impacted Files	Coverage Δ
fortls/objects.py	`83.04% <90.00%> (+0.08%)`	⬆️
fortls/langserver.py	`82.36% <100.00%> (-0.81%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

gnikit · 2022-11-01T00:40:22Z

Thanks for the PR and the issue @plevold. I will try and have a look this week and leave a review. 👍

gnikit · 2022-11-01T00:51:50Z

The thing that you need to be extra careful is how Doxygen and modified FORD documentation can look very similar.
for example in the issue you posted you added this code

    !> Some documentation here?
    type :: a_t
        !> Or here?
       integer :: var    ! what if there is variable? is `!> Or here?` considered its docstrings
    end type

What do we expect the behaviour of the Document parser to be prefer 1 of the 2 docstrings (and if so why) or concatenate?
I remember looking into this a few years back but I don't recall what the syntax should be.

FYI
We can do A LOT with fortls in terms documentation. I have intentionally absteint, because users tend to get annoyed when something changes without backwards compatibility, but fortls could both auto generated docs and convert FORD to Doxygen and vice versa.

plevold · 2022-11-02T07:28:46Z

The thing that you need to be extra careful is how Doxygen and modified FORD documentation can look very similar.
for example in the issue you posted you added this code

Agree and I see now that my initial issue description was incorrect in that regard. My changes actually doesn't do anything to the docstring parsing itself. The docstrings were already parsed correctly and added to the module and type objects. The only thing I had to add was to return the docstring in hover requests for module and type names.

One thing I noticed during my testing is that Doxygen style comments (aka. pending_doc) will get added to the next Scope even if there are other expressions in between. So for example, if somebody tries to add a docstring to an expression:

subroutine sub_before
    integer :: i

    !> Trying to add a docstring to an expression
    i = 1
end subroutine


subroutine sub_no_doc
end subroutine

It will actually be added to the sub_no_doc subroutine below. I might try to fix this at some point, but I think that belongs to a separate PR.

gnikit

Looks good, a couple of things are needed for this to be merged.

rebase to master so the Markdown changes are ported
use f-strings where possible instead of format() or "str" + "val"
add unittests for the uncovered lines

The issue that you noted with Doxygen style comments being added to the wrong scope should also be ideally addressed since it can lead to quite a bit of a nightmare. Might be better if done in a separate PR though.

I will have some time this week after I finish the work with autocopmletions and Markdown to test your version of fortls locally and see if we need to add anything else in the hover signature and/or signature help and completions.

fortls/objects.py

gnikit · 2022-11-07T00:12:42Z

fortls/objects.py

+    def get_hover(self, long=False, include_doc=True, drop_arg=-1):
+        keywords = ""
+        if self.abstract:
+            keywords += ", ABSTRACT"
+        if self.inherit:
+            keywords += ", EXTENDS({})".format(self.inherit)
+        hover = "TYPE{} :: {}".format(keywords, self.name)
+        if include_doc:
+            doc_str = self.get_documentation()
+            if doc_str is not None:
+                hover += "\n" + doc_str
+        return hover, long


use fstrings and consider using a list for keywords.
I think that explicit newline character might not be correct now that Markdown is being used for hover messages.

Also, we need to provide some unit tests for the non-covered lines.

fortls/langserver.py

plevold · 2022-11-16T15:35:56Z

@gnikit thanks for the feedback and sorry about the delay! My schedule has been incredibly unpredictable the last month or so.

I think I now have resolved all the items you pointed out. As you noticed I also added #237 regarding the potential pitfalls with pending_doc.

codecov · 2022-11-16T15:37:23Z

Codecov Report

Merging #212 (09a75d8) into master (9e5f174) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 09a75d8 differs from pull request most recent head 87cd3cc. Consider uploading reports for the commit 87cd3cc to get more accurate results

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
- Coverage   86.76%   86.76%   -0.01%     
==========================================
  Files          12       12              
  Lines        4542     4489      -53     
==========================================
- Hits         3941     3895      -46     
+ Misses        601      594       -7

Impacted Files	Coverage Δ
fortls/langserver.py	`84.21% <100.00%> (-0.10%)`	⬇️
fortls/objects.py	`83.60% <100.00%> (-0.01%)`	⬇️
fortls/parse_fortran.py	`89.01% <0.00%> (-0.01%)`	⬇️
fortls/intrinsics.py	`96.20% <0.00%> (ø)`
fortls/regex_patterns.py	`100.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gnikit · 2022-11-20T20:38:01Z

There are some stuff that I need to change, style in fortran code, some changes you made that remove coverage, so if it's okay with you, I will push directly on this branch.

plevold · 2022-11-20T20:59:59Z

Sure, no problem

Fixes fortran-lang#208

gnikit · 2022-12-07T23:54:37Z

I have no idea what's going on with the coverage reports in this PR.

I have started a new one (#246) with your commit cherry-picked, but edited to undo some of the breaking changes like disabling autoupdating and ' vs ". I will inspect the new coverage reports and see if any coverage lines are being removed this time around

plevold requested a review from gnikit as a code owner October 30, 2022 11:37

plevold force-pushed the docstrings-for-modules-and-types branch 2 times, most recently from eca301d to 60e0646 Compare October 30, 2022 11:46

gnikit requested changes Nov 7, 2022

View reviewed changes

gnikit reviewed Nov 7, 2022

View reviewed changes

fortls/langserver.py Outdated Show resolved Hide resolved

plevold mentioned this pull request Nov 16, 2022

Doxygen style comments are sometimes picked up in the wrong place #237

Open

plevold force-pushed the docstrings-for-modules-and-types branch from 60e0646 to 30c80f1 Compare November 16, 2022 15:32

gnikit force-pushed the docstrings-for-modules-and-types branch from 09a75d8 to 3e63fc1 Compare December 4, 2022 23:08

feat: Add hover info for modules and types including any docstring

87cd3cc

Fixes fortran-lang#208

gnikit force-pushed the docstrings-for-modules-and-types branch from 3e63fc1 to 87cd3cc Compare December 4, 2022 23:28

gnikit mentioned this pull request Dec 7, 2022

feat: add hover for types and modules #246

Merged

gnikit closed this Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add hover info for modules and types including any docstring #212

feat: Add hover info for modules and types including any docstring #212

plevold commented Oct 30, 2022

codecov bot commented Oct 30, 2022 •

edited

Loading

gnikit commented Nov 1, 2022

gnikit commented Nov 1, 2022

plevold commented Nov 2, 2022

gnikit left a comment

gnikit Nov 7, 2022

plevold commented Nov 16, 2022

codecov bot commented Nov 16, 2022 •

edited

Loading

gnikit commented Nov 20, 2022

plevold commented Nov 20, 2022

gnikit commented Dec 7, 2022

feat: Add hover info for modules and types including any docstring #212

feat: Add hover info for modules and types including any docstring #212

Conversation

plevold commented Oct 30, 2022

codecov bot commented Oct 30, 2022 • edited Loading

Codecov Report

gnikit commented Nov 1, 2022

gnikit commented Nov 1, 2022

plevold commented Nov 2, 2022

gnikit left a comment

Choose a reason for hiding this comment

gnikit Nov 7, 2022

Choose a reason for hiding this comment

plevold commented Nov 16, 2022

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

gnikit commented Nov 20, 2022

plevold commented Nov 20, 2022

gnikit commented Dec 7, 2022

codecov bot commented Oct 30, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading