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

feat: Change hover to use MarkupContent #213

Merged
merged 15 commits into from
Nov 6, 2022
Merged

feat: Change hover to use MarkupContent #213

merged 15 commits into from
Nov 6, 2022

Conversation

gnikit
Copy link
Member

@gnikit gnikit commented Oct 30, 2022

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

Changes:

  • Changes the hover signature of variables from "integer, pointer" --> "integer, pointer :: val"
  • Changes the Signature help message for Functions/Subroutines to use Markdown

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

TODO:

  • Fix doxygen parser. Parse @param[in|out] as --> arg -- desc'
  • replace hardwired fortran90 in objects.py to "{langid}" and then string.from(langid=self.hover_language)

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

Fixes #45
@gnikit
Copy link
Member Author

gnikit commented Oct 30, 2022

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

@codecov
Copy link

codecov bot commented Oct 31, 2022

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%) ⬆️
fortls/objects.py 83.07% <89.65%> (+0.11%) ⬆️
fortls/helper_functions.py 97.73% <100.00%> (+0.07%) ⬆️
fortls/intrinsics.py 95.55% <100.00%> (ø)
fortls/parse_fortran.py 88.71% <100.00%> (+0.37%) ⬆️

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

gnikit and others added 3 commits November 1, 2022 16:21
Fixes bug(parser): multiline scope registration assigns end of line not start of line as scope start #217
updates:
- [github.com/asottile/pyupgrade: v3.0.0 → v3.1.0](asottile/pyupgrade@v3.0.0...v3.1.0)
@plevold
Copy link
Contributor

plevold commented Nov 2, 2022

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

gnikit commented Nov 2, 2022

  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
Copy link
Contributor

plevold commented Nov 2, 2022

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.

The hover response included a snippet for a signature request which
is not how signature help requests are dealt.
Now the documentation of the procedure will
be interpreted as Markdown similar to hover
@gnikit gnikit merged commit 689f044 into master Nov 6, 2022
@gnikit gnikit deleted the gnikit/issue45 branch November 6, 2022 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants