fortran-lang / fortls

fortls - Fortran Language Server
https://fortls.fortran-lang.org/
MIT License
258 stars 41 forks source link

feat: Change hover to use MarkupContent #213

Closed gnikit closed 2 years ago

gnikit commented 2 years ago

MarkedString[] has been deprecated so we switch to full Markdown syntax for hover messages

Changes:

Fixes #45 Fixes #70 if client permits it (vs code does not allow mathjax in markdown) Fixes #214 Fixes #217

TODO:

gnikit commented 2 years ago

@plevold might want to have a look on this. I still need to fix the Doxygen parsing of docstrings

codecov[bot] commented 2 years ago

Codecov Report

Merging #213 (6f531f2) into master (abd5d34) will increase coverage by 0.28%. The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   86.03%   86.31%   +0.28%     
==========================================
  Files          12       12              
  Lines        4439     4451      +12     
==========================================
+ Hits         3819     3842      +23     
+ Misses        620      609      -11     
Impacted Files Coverage Δ
fortls/langserver.py 83.60% <88.23%> (+0.43%) :arrow_up:
fortls/objects.py 83.07% <89.65%> (+0.11%) :arrow_up:
fortls/helper_functions.py 97.73% <100.00%> (+0.07%) :arrow_up:
fortls/intrinsics.py 95.55% <100.00%> (ø)
fortls/parse_fortran.py 88.71% <100.00%> (+0.37%) :arrow_up:

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

plevold commented 2 years ago

@gnikit looks very nice indeed! I've noticed two issues that would make it even better:

  1. The argument documentation list gets put at the bottom. When I write long docstrings for procedures it's easy to end up in a situation where I don't see the signature when reading the argument documentation list. I'd suggest putting the list above the docstring.

  2. Whitespace at the beginning of docstring lines gets trimmed. This makes code examples with indentation look weird. For example:

!> Generate an error report with the given message as root cause.
!! Use this to create general application errors which don't need to be
!! identified programatically. See
!!
!! ### Example:
!! ```fortran
!! subroutine ask_hal_9000(error)
!!     use error_handling, only: error_t, fail
!!     class(error_t), intent(out) :: error
!!
!!     error = fail("I'm sorry Dave, I can't do that")
!! end subroutine
!! ```
pure module function fail_message(message) result(error)
    !> Message to fail with
    character(len=*), intent(in) :: message
    type(error_report_t) :: error
end function

Gets turned into this hover: image

I checked and this behaviour is present in the current fortls release as well, but I'm not sure where the trimming takes place.

gnikit commented 2 years ago
  1. The argument documentation list gets put at the bottom. When I write long docstrings for procedures it's easy to end up in a situation where I don't see the signature when reading the argument documentation list. I'd suggest putting the list above the docstring.

Even though I too agree that this is a more intuitive way to display documentation (having had my start with C++ where docs are on top), you will see that both the Python, C++, JS and TS extension display hover messages the same way. Not sure what the justification for this is but I would aim to keep it consistent.

  1. Whitespace at the beginning of docstring lines gets trimmed. This makes code examples with indentation look weird. For example:

We actually strip all code lines in a few places. It makes easier to parse if you don't have to worry about white spaces.

Then to create new lines in hover Markdown we use a combination of spaces and newline characters, else the docstring would be too spaced out.

In theory what you're saying should be relatively easy to fix only because it's the leading whitespace and not the trailing ones. Although reality might be a bit different, I'll have a look.

plevold commented 2 years ago

you will see that both the Python, C++, JS and TS extension display hover messages the same way. Not sure what the justification for this is but I would aim to keep it consistent.

I had not noticed that! I agree that consistency is a good idea. Also, if somebody really want an argument list at the top they could just include it in the main docstring instead so I don't think it needs to be changed.