Add hover functionality for dependencies in Gemfile #1279

bravehager · 2024-01-04T20:17:03Z

Motivation

I've noticed that engineers are often inclined to litter their Gemfile with comments that summarize each and every gem dependency (often just pulling out the summary straight from the gemspec). This feels like a job for an LSP.

Implementation

Check for gem dependency definitions and add hover based on gem specification if applicable.

Overall, this PR could probably use some additional refactoring and test cases, but I wanted to present the concept for validation first.

Some open questions:

Do we display the summary, description or both?
Is there additional information in the spec worth displaying?
Should this feature be hidden behind a setting?

Automated Tests

Added a simple test case.

Manual Tests

Open the branch
Confirm the hover functionality for dependencies in Gemfile

vinistock

Really neat idea. Thanks for proposing this. I left some comments, but they are mostly details

lib/ruby_lsp/requests/hover.rb

test/requests/hover_expectations_test.rb

andyw8 · 2024-01-08T20:17:25Z

When testing this, I noticed problems with some gemspecs that use heredocs, e.g.:

https://github.com/lsegal/yard/blob/2d197a381c5d4cc5c55b2c60fff992b31c986361/yard.gemspec#L7-L12

Since the lines contain leading whitespace, the markdown treats that as code, so it renders like this:

Really the gemspecs should be using <<~ rather than <<- but I think we should handle it by stripping the leading whitespace.

andyw8 · 2024-01-08T20:22:29Z

Do we display the summary, description or both?

I think we should match rubygems.org:

https://github.com/rubygems/rubygems.org/blob/056a10c1634ae77a2dd418bf7303b06ccd1b9267/app/models/version.rb#L255

bravehager · 2024-01-08T21:42:32Z

lib/ruby_lsp/requests/hover.rb

+        info = [spec.description, spec.summary, "This rubygem does not have a description or summary."].find do |text|
+          !text.nil? && !text.empty?
+        end


I don't think we can use #present? here since it's an Active Support extension—not the prettiest thing in the world but I think it's roughly the same in terms of functionality based on this guide.

bravehager · 2024-01-08T21:43:24Z

@vinistock Thanks for the review! I think I incorporated all the feedback, let me know if there's anything else missing.

bravehager · 2024-01-08T23:54:43Z

When testing this, I noticed problems with some gemspecs that use heredocs, e.g.:

https://github.com/lsegal/yard/blob/2d197a381c5d4cc5c55b2c60fff992b31c986361/yard.gemspec#L7-L12

Since the lines contain leading whitespace, the markdown treats that as code, so it renders like this:

I think this actually points out two issues, one being the incorrect use of heredocs and the other being that the content itself isn't actually markdown anyways.

I see a couple options:

Truncate the leading whitespace on each line. Works for me, but AFAIK there's no way to distinguish between a string literal that uses a non-squiggly heredoc vs. has intentional leading whitespace (if there is, I think this is a great solution without many trade-offs).
Use plaintext for the entire hover response.
Keep using markdown but wrap only the summary/description so it doesn't get formatted. I tried using div and pre tags, but so far I could only get a text code fence to work:

Doesn't solve the original issue with heredocs but I think it might preserve the intentionality better in some cases, e.g. Minitest:

I'm fine w/ any solution (or something else entirely, or a combination of them).

andyw8 · 2024-01-09T14:02:37Z

Option 1 makes sense to me. I can't think of a situation where there would be intentional leading whitespace in the description.

(Minitest is an odd one, I wouldn't worry too much about it since Rubygems.org doesn't render it well either: https://rubygems.org/gems/minitest/versions/5.20.0).

andyw8

Tried the latest update and YARD now displays correctly. Great work!

lib/ruby_lsp/requests/hover.rb

vinistock · 2024-01-09T19:51:43Z

lib/ruby_lsp/requests/hover.rb

+        # Remove leading whitespace if a heredoc was used for the summary or description
+        info = info.gsub(/^ +/, "")


I understand the desire to make the presentation look good, but I wonder if we should instead create incentive for authors to fix their descriptions.

For example, YARD clearly wanted to use the ~ version of heredocs given that the entire description is indented at the same level. Feels like changing that in YARD would also ensure other tools display it correctly, rather than trying to account for that in the LSP.

Yeah, it is kind of smoothing over an issue upstream. I can see the merits of either. We could also move the version into a Code Lens (or do away with it entirely), and keep the hover content restricted to the plaintext summary/description content.

Hmm, even the example in the Rubygems docs uses <<- with leading whitesapce:

https://guides.rubygems.org/specification-reference/#description

I can open a PR on https://github.com/rubygems/guides to use the squiggly heredoc. I would assume the docs pre-date the newer syntax itself?

Yeah, that would be a helpful.

I still think we should strip the leading whitespace though, since some gems will never get around to updating, and it results in a disjointed experience for the user.

Thinking about this more, even if it was supported in all non-EOL Ruby versions, it might actually restrict the gem from being installed on older versions due to invalid syntax, which might not be a reasonable trade-off.

Opened rubygems/guides#352 to address it in the RubyGems guides.

Thinking about this more, even if it was supported in all non-EOL Ruby versions, it might actually restrict the gem from being installed on older versions due to invalid syntax, which might not be a reasonable trade-off.

The .gemspec file is not actually part of the distributed .gem package so I don't think it would be a problem.

TIL they're stored separately. It looks like the heredoc itself actually gets stripped out anyways and converted to a plain-old string literal (looking at my local copy of ~/.gem/ruby/3.3.0/specifications/yard-0.9.34.gemspec).

Alright, let's ship this PR and we can always revisit the blank spaces thing later. No need to hold it back.

bravehager · 2024-01-09T20:26:04Z

test/requests/hover_expectations_test.rb

@@ -181,7 +181,6 @@ def test_hovering_over_gemfile_dependency

    assert_includes(response.contents.value, spec.name)
    assert_includes(response.contents.value, spec.version.to_s)
-    assert_includes(response.contents.value, spec.description)


I think CI was failing on this line. Since we're stripping the description, I don't think we should assert on it anyways.

vinistock

Thank you for the contribution!

vinistock · 2024-01-10T16:07:53Z

lib/ruby_lsp/requests/hover.rb

+        # Remove leading whitespace if a heredoc was used for the summary or description
+        info = info.gsub(/^ +/, "")


Alright, let's ship this PR and we can always revisit the blank spaces thing later. No need to hold it back.

bravehager requested a review from a team as a code owner January 4, 2024 20:17

bravehager requested review from egiurleo and KaanOzkan January 4, 2024 20:17

KaanOzkan requested review from vinistock and removed request for KaanOzkan January 4, 2024 21:26

vinistock reviewed Jan 8, 2024

View reviewed changes

bravehager added 5 commits January 8, 2024 16:04

Add hover functionality to dependencies in Gemfile

d4f7962

Add test case

09bb766

Always use currently loaded spec

ab91a34

Incorporate feedback

bf9f939

Add additional test cases

f23253b

bravehager force-pushed the gem-hover branch from 81bcb71 to f23253b Compare January 8, 2024 21:39

bravehager commented Jan 8, 2024

View reviewed changes

bravehager requested a review from vinistock January 8, 2024 21:42

Remove unused instance variable

806e94d

bravehager added 2 commits January 9, 2024 09:18

Remove leading whitespace from gem info

fb7dec5

Use safe navigation

a8bc251

andyw8 approved these changes Jan 9, 2024

View reviewed changes

andyw8 reviewed Jan 9, 2024

View reviewed changes

lib/ruby_lsp/requests/hover.rb Outdated Show resolved Hide resolved

Cast info as String

6f69e7b

vinistock reviewed Jan 9, 2024

View reviewed changes

Skip assertion on gem description

50c5f85

bravehager commented Jan 9, 2024

View reviewed changes

bravehager mentioned this pull request Jan 9, 2024

Use squiggly heredoc for gem description example rubygems/guides#352

Closed

egiurleo removed their request for review January 9, 2024 22:33

bravehager mentioned this pull request Jan 10, 2024

Use squiggly heredocs in Gem::Specification#description documentation, so it doesn't add leading whitespace rubygems/rubygems#7373

Merged

4 tasks

vinistock approved these changes Jan 10, 2024

View reviewed changes

vinistock merged commit a68fceb into Shopify:main Jan 10, 2024
16 of 19 checks passed

st0012 added the enhancement New feature or request label Jan 11, 2024

vinistock mentioned this pull request Jan 12, 2024

CodeLens support for minitest spec-style tests #1265

Merged

vinistock mentioned this pull request Jan 30, 2024

Move open remote code lenses into hover instead #1339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hover functionality for dependencies in Gemfile #1279

Add hover functionality for dependencies in Gemfile #1279

bravehager commented Jan 4, 2024

vinistock left a comment

andyw8 commented Jan 8, 2024 •

edited

Loading

andyw8 commented Jan 8, 2024

bravehager Jan 8, 2024

bravehager commented Jan 8, 2024

bravehager commented Jan 8, 2024

andyw8 commented Jan 9, 2024

andyw8 left a comment

vinistock Jan 9, 2024

bravehager Jan 9, 2024 •

edited

Loading

andyw8 Jan 9, 2024 •

edited

Loading

bravehager Jan 9, 2024

andyw8 Jan 9, 2024

bravehager Jan 9, 2024

bravehager Jan 9, 2024

andyw8 Jan 9, 2024

bravehager Jan 9, 2024

vinistock Jan 10, 2024

bravehager Jan 9, 2024

vinistock left a comment

vinistock Jan 10, 2024

		# Remove leading whitespace if a heredoc was used for the summary or description
		info = info.gsub(/^ +/, "")

Add hover functionality for dependencies in Gemfile #1279

Add hover functionality for dependencies in Gemfile #1279

Conversation

bravehager commented Jan 4, 2024

Motivation

Implementation

Automated Tests

Manual Tests

vinistock left a comment

Choose a reason for hiding this comment

andyw8 commented Jan 8, 2024 • edited Loading

andyw8 commented Jan 8, 2024

Choose a reason for hiding this comment

bravehager commented Jan 8, 2024

bravehager commented Jan 8, 2024

andyw8 commented Jan 9, 2024

andyw8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bravehager Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

andyw8 Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Jan 8, 2024 •

edited

Loading

bravehager Jan 9, 2024 •

edited

Loading

andyw8 Jan 9, 2024 •

edited

Loading