Introduce Java doc comment rendering #3472

vemv · 2023-09-21T12:02:41Z

Introduces cider-docstring.el which is able to render "fragments" (a mixture of plain text and html) into a propertized string.

Starts using such rendered strings in:

Company (and with it, Corfu, since internally it queries Company stuff)
- For both, the output is always trimmed to 20 lines (customizable)
- The trimming is smart as it can perform several attempts based on 3 sources: The first sentence, the full body, and the block tags (i.e. the returns/throws/params metadata)
Eldoc
- Here we always favor only rendering the first sentence only
cider-doc anciliary buffers
- as shown e.g. when pressing f1 while using Company
- Here, we favor short rendering only if using Company
Mouse tooltips.

For best results, we also start sending a Compliment context to eldoc and info requests.

The context is slightly different than usual: instead of (.foo__prefix__ bar), we have to send (__prefix__ bar), because we're not requesting a completion, but for an inferred class.

Therefore a cider-completion-get-info-context-at-point defun is introduced.

Two new .el files are introduced:

cider-context, for extracting the context
cider-docstring, for docstring rendering

This way we avoid cyclical deps.

bbatsov · 2023-09-21T12:42:20Z

Company (and with it, Corfu, since internally it queries Company stuff)

I don't get this - Corfu doesn't use anything company-specific to my knowledge.

cider-context, for extracting the context

I have some doubts about the extraction of this. I get why you did it, but it looks relatively weird on its own, given the significant connection to completion. Perhaps at least we can name the file cider-completion-context so it's clearer what this is?

bbatsov · 2023-09-21T12:56:15Z

I think we should also expand the completion/docs documentation to mention some of those changes.

vemv · 2023-09-21T13:56:55Z

I don't get this - Corfu doesn't use anything company-specific to my knowledge.

It queries these for showing docstrings:

https://github.com/minad/corfu/blob/cc244c54b392ce4d27ed166730efde4f4d01a07f/extensions/corfu-echo.el#L88C41-L88C52

https://github.com/minad/corfu/blob/cc244c54b392ce4d27ed166730efde4f4d01a07f/extensions/corfu-popupinfo.el#L216C13-L216C62

Perhaps at least we can name the file cider-completion-context so it's clearer what this is?

SGTM!

I think we should also expand the completion/docs documentation to mention some of those changes.

Will do 👍

vemv · 2023-09-22T19:34:58Z

@bbatsov ready again.

Besides from Company, cider-doc and eldoc, I've been trying these docstrings over the mouse tooltips as well.

Looking correct and concise in all of those.

bbatsov · 2023-09-25T05:17:01Z

Eldev

@@ -11,6 +11,12 @@
 (eldev-add-loading-roots 'test "test/utils")
 (eldev-add-extra-dependencies 'runtime '(:package logview :optional t))

+;; slightly increase the maximum (applies to checkdoc and the byte compiler alike)
+(setq byte-compile-docstring-max-column 100)


The Emacs maintainers advised against doing this, that's why I had changed it. Normally it's best to have development settings in .dir-locals.el (you can have them nested in various folders)

It didn't work otherwise though 😞

Hmm, I can't imagine how - after all .dir-locals are applied to the entire directory. Unless Eldev is suppressing those somehow.

I don't know

There's no mention of .dir-locals in the Eldev repo, which might be a signal that it isn't considered as a config mechanism (it does mention four levels of configuration in the readme)

@doublep Does Eldev ignore dir-locals intentionally? If so, I guess we should have the config in both places.

No, it simply doesn't do anything special. Which means they are not activated through some automatic magic.

it does mention four levels of configuration in the readme

Those don't include .dir-locals. All ways of configuration are Eldev-specific, but let you evaluate arbitrary Elisp code, not just set variables to constant values.

bbatsov · 2023-09-25T05:18:56Z

test/cider-docstring-tests.el

+
+(describe "cider--render-docstring"
+  (it "A large corpus of fragments (as produced by Orchard) can be rendered using `shr' without raising errors"
+    (dolist (class '("Thread" "Object" "File" "String"))


I'm thinking this setup code can be extracted and you can have several more granular tests, because now it's a bit harder to follow what exactly is the purpose of the test given it has a bunch of asserts and expects in it. I think it's usually better to have several tests that check for fewer things each than fewer tests that check for more things.

Mmm, the it "A large corpus of fragments (as produced by Orchard) can be rendered using shr' without raising errors"` sets out a reasonable context as for what to expect.

I essentially want a smoke test. HTML renderding seems somewhat delicate. With a large corpus of html fragments, we can be reasonably sure that we aren't hitting some weird corner of the shr library.

The asserts help ensuring that actually iterating stuff.

A specific suggestion would be most welcome. Perhaps I can simply extract test helpers so that the main thing tested is clearer? Other than that, I'm not sure (why) I'd structurally change this.

I thought you can just generate the large corpus of fragments upfront and test the different things you wanted to verify in it separately in individual tests so it's clearer what's considered correct behavior. The test is not particularly complex, so that's not a big deal.

My main concern is the usage of cl-assert in the test, which I don't quite get, given that you already have expect.

I use cl-assert <-> expect in Elisp just like how I use assert <-> is in clojure: an assert ensures the sanity of the test itself, but does is not concerned with the SUT.

This separation can help seeing which if the two intents one is expressing.

Finally, cl-assert appears to fail more clearly under buttercup.

vemv · 2023-09-27T20:51:46Z

Eldev

@@ -11,6 +11,12 @@
 (eldev-add-loading-roots 'test "test/utils")
 (eldev-add-extra-dependencies 'runtime '(:package logview :optional t))

+;; slightly increase the maximum (applies to checkdoc and the byte compiler alike)
+(setq byte-compile-docstring-max-column 100)


I don't know

There's no mention of .dir-locals in the Eldev repo, which might be a signal that it isn't considered as a config mechanism (it does mention four levels of configuration in the readme)

vemv · 2023-09-27T20:52:41Z

.github/workflows/test.yml

@@ -14,6 +14,7 @@ jobs:
      matrix:
        os: [macos-latest, ubuntu-latest, windows-latest]
        emacs_version: ['26.3', '27.2', '28.2', '29.1']
+        java_version: ['11', '17']


I introduced a JVM matrix (soon to be expanded with JDK 21)

vemv · 2023-09-27T20:55:00Z

.github/workflows/test.yml

+    - name: Run tests that need enrich-classpath
+      if: "!startsWith(matrix.os, 'windows')"
+      run: |
+        cd dev; ../clojure.sh clojure -M:gen; cd -


now the test/File.edn files are generated programatically.

As a nice side-effect, we get to exercise our clojure.sh script, and with it, enrich-classpath.

Both get an extra exercise given the JVM matrix.

We could build more tests like this in a future - nice middle ground between reliable and realistic tests (since there's no nrepl connection - only data).

bbatsov · 2023-09-28T08:33:20Z

cider-doc.el

-    (cider-docview-render (cider-make-popup-buffer cider-doc-buffer nil 'ancillary) symbol info)))
+    (cider-docview-render (cider-make-popup-buffer cider-doc-buffer nil 'ancillary) symbol info shorter)))
+
+(defun cider-create-shorter-doc-buffer (symbol)


I wonder if "compact" or "abridged" don't reflect better what we're doing here. "shorter" sounds a bit confusing to me.

bbatsov · 2023-09-28T08:33:32Z

cider-doc.el

-  "Populates *cider-doc* with the documentation for SYMBOL."
+(defun cider-create-doc-buffer (symbol &optional shorter)
+  "Populates *cider-doc* with the documentation for SYMBOL,
+favoring a SHORTER format if specified."


This can be "compact" format or something similar IMO.

vemv requested a review from bbatsov September 21, 2023 12:05

Base automatically changed from cider-nrepl-0-38-0 to master September 21, 2023 17:01

vemv force-pushed the doc-fragments branch from 3913251 to 90f942a Compare September 22, 2023 16:07

vemv marked this pull request as ready for review September 22, 2023 19:32

vemv force-pushed the doc-fragments branch from b40c869 to c030370 Compare September 23, 2023 09:31

bbatsov reviewed Sep 25, 2023

View reviewed changes

vemv force-pushed the doc-fragments branch from 6e74eca to f92fa27 Compare September 27, 2023 20:49

vemv commented Sep 27, 2023

View reviewed changes

bbatsov reviewed Sep 28, 2023

View reviewed changes

vemv added 15 commits October 6, 2023 09:04

Introduce Java doc comment rendering

43f574c

Shorten cider-docview-render-info when called from Company

d0f0819

Rename cider-context -> cider-completion-context

e5cf775

Extract cider-completion--bounds-of-non-string-symbol-at-point

4074afa

Document new features

f8286c9

Refine integration with tooltips

8c336c2

Introduce cider-docstring--dumb-trim

c91a296

Fix linting

aab2ce9

Tweak alignment for tooltips

97b1761

Add ellipsis to trimmed strings

9084c4d

Remove temp flag

7331b29

cider-docstring--dumb-trim: clean up other whitespace

9b3ce28

Don't render see also when :shorter

fe71782

Set a smarter default for cider-company-docsig-render-docstring

8850264

Document company-show-doc-buffer integration

5a438f2

vemv added 10 commits October 6, 2023 09:04

nil safe

d18baef

cider-docview-render-info: format args better

73e9680

Make completion contexts work in repls

f1d5257

Add cider-docstring-tests.el

7e77c20

Generate test/*.edn files programatically

2d831b1

Observe annotated-arglists

a04f436

Add more tests

69fd34a

Discard cider-company-docsig-render-docstring defcustom

b9dcc30

Rename shorter -> compact

6ab5381

Get the full context for eldoc

548a57b

vemv force-pushed the doc-fragments branch from f92fa27 to 548a57b Compare October 6, 2023 17:35

vemv added 2 commits October 6, 2023 19:36

Bump Orchard

952bbbc

Refine Company docs

e0fd855

vemv merged commit ee1975d into master Oct 6, 2023
21 of 34 checks passed

vemv deleted the doc-fragments branch October 6, 2023 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Java doc comment rendering #3472

Introduce Java doc comment rendering #3472

vemv commented Sep 21, 2023 •

edited

bbatsov commented Sep 21, 2023

bbatsov commented Sep 21, 2023

vemv commented Sep 21, 2023

vemv commented Sep 22, 2023

bbatsov Sep 25, 2023

vemv Sep 25, 2023

bbatsov Sep 27, 2023

vemv Sep 27, 2023 •

edited

bbatsov Sep 28, 2023

doublep Sep 28, 2023

bbatsov Sep 25, 2023

vemv Sep 25, 2023

bbatsov Sep 28, 2023

vemv Oct 6, 2023

vemv Sep 27, 2023 •

edited

vemv Sep 27, 2023

vemv Sep 27, 2023 •

edited

bbatsov Sep 28, 2023

bbatsov Sep 28, 2023

Introduce Java doc comment rendering #3472

Introduce Java doc comment rendering #3472

Conversation

vemv commented Sep 21, 2023 • edited

bbatsov commented Sep 21, 2023

bbatsov commented Sep 21, 2023

vemv commented Sep 21, 2023

vemv commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv Sep 27, 2023 • edited

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

vemv Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv commented Sep 21, 2023 •

edited

vemv Sep 27, 2023 •

edited

vemv Sep 27, 2023 •

edited

vemv Sep 27, 2023 •

edited