General stability fixes and arglist support in completion hover #375

cfehse · 2019-10-04T15:04:40Z

Fixes #238, #366, #367, #368

Introduces the following change(s):

Added the argument list to HoverProvider and CalvaCompletionItemProvider.
Added a mechanism to create the namespace from a file in the REPL if that namespace does not exist in the REPL. This is necessary because if the namespace with which the nrepl command 'info' is called (the namespace of the file) does not exists no data is returned.
Added activation events workspaceContains:**/project.clj, workspaceContains:**/shadow-cljs.edn and workspaceContains:**/deps.edn to allow the extension to be activated faster.
Improved the state.initProjectDir() function to use vscode.workspace.workspaceFolders![0] as a second starting point in case no file is open in the workspace is not multi-root. This conforms to our wiki entry about the supported workspace layout. Made the function robust if no document is defined.
Made the code robust in case a jack-in task exists prematurely an no REPL client is connected.
Catch exception during fs.watch() in executeJackInTask(). These exception is according to the fs team by design if a given directory to watch does not exists or the underlying filesystem (network filesystems for example) does not support the operation. During the jack-in process this may occur if the user selects the wrong type of project - e.g. Leiningen + shadow-cljs if there is no shadow support in the project. In case the exception is detected, the jack-in task is canceled.
Added the file and untitled schemes to all document selectors. These are the vscode supported schemes. The addition prevents the vscode warning message Extension '<extension>' uses a document selector without scheme. Learn more about....

Tested on

Windows
Linux
macOS

I have:

Read How to Contribute.
Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
Referenced the issue I am fixing/addressing in a commit message for the pull request.
- If I am fixing the issue, I have used GitHub's fixes/closes syntax
Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

Ping: @PEZ, @kstehn

See PR description for details.

The link to About-Calva-Jack-in should be (I believe!) to Connect-Calva-to-Your-Project

Fix broken link to Wiki

cfehse · 2019-10-05T11:45:03Z

@PEZ This is ready for testing/merging.

PEZ · 2019-10-05T12:14:51Z

Added a mechanism to load a file into the REPL if the namespace of the file does not exist in the REPL. This is necessary because if the namespace with which the nrepl command 'info' is called (the namespace of the file) no data is returned. So the file must be loaded to retrieve data. Another advantage of this approach is that if the file was loaded information of the elements contained in the file can be displayed as well.

This is a generally good idea. There are many first time experiences with Calva that get a bit strange because the namespaces are not loaded. However, I think this needs to be a setting. Much as I try to avoid settings, there are quite a few users who have opposed to make namespace loading the default behaviour. They have workflows where their namespaces contain code that can take very long to run, and will be hit badly if Calva runs the code just because a file is opened. I think this setting should default to loading the namespaces.

PEZ · 2019-10-05T12:20:19Z

Added activation events workspaceContains:**/project.clj and workspaceContains:**/shadow-cljs.edn to allow the extension to be activated faster.

Can I haz deps.edn added too?

Maybe **/*.{clj,edn}* would work, even...

cfehse · 2019-10-05T12:30:53Z

@PEZ
If not loading the file then perhaps just evaluate an "empty file" containing the correct namespace to create the namespace in the repl. Without the namespace known to the repl there will be no information even about the core clojure elements.

Another option is to make the info call in the user namespace if the namespace of the open file does not exist in the repl.

PEZ · 2019-10-05T12:32:36Z

Tested the autoload namespace thing now, and love it.

However, in a .cljc file, it only seems to do it for the clj repl. If I toggle the file to be backed by the cljs REPL, then I need to manually load the file. (We can add this as a separate issue if it looks tricky to solve now.)

PEZ · 2019-10-05T12:37:52Z

I agree that we might do better than just skip loading anything. I still suggest that for now we add a setting that defaults to true and if it is false then the user has to load the file, just as today. We can add an issue for refining it. But it is your call, since your PR. 😄

cfehse · 2019-10-05T12:48:13Z

@PEZ

Take a good look at all the changes so I can take care of the PR on Monday.

PEZ · 2019-10-05T12:51:44Z

Tested:

Hover. Works like a charm.
Autoloading file. See above comments.
Activation events, see above comments. (Can be shipped as is, but a bit strange to leave tools.deps users hanging. 😄
Jack-in in single-root and multi-root namespaces. I look forward to updating the wiki on this!

src/providers/hover.ts

PEZ · 2019-10-05T13:02:18Z

I've skimmed the code changes now. Looks good to me. What do you say, @kstehn ?

src/nrepl/jack-in.ts

kstehn · 2019-10-05T13:12:09Z

src/nrepl/jack-in.ts

        // REPL window open commands if needed.
-        commands["Disconnect from the REPL server"] = "calva.disconnect";
+        commands["Disonnect from the REPL server"] = "calva.disconnect";


That should be Disconnect or not?

@kstehn Corrected that.

kstehn · 2019-10-05T13:15:53Z

src/providers/hover.ts

+            if(client) {
+                await util.loadFileIfNamespaceNotExist(document);
+                let res = await client.info(ns, text);
+                // I really don't not why result.arglists-str does 


The reason for that is the -
Otherwise it would not be possible to do something like this
res.a - res.b i think

@kstehn Okay. gg

slipset · 2019-10-06T13:05:08Z

src/providers/hover.ts

        // Format the actual docstring
-        if (doc) {
-            result += doc.replace(/\s\s+/g, ' ');
+        if (documentation && documentation != "") {


Jumping in here, so please disregard.
This is basically the same as https://github.com/BetterThanTomorrow/calva/pull/375/files#diff-9205a2ccff45e74e74a392042d83efe8R25 Could/should it be extracted into a docUtils (or some such, naming is hard)?

slipset · 2019-10-06T13:09:09Z

src/providers/hover.ts

-        if (this.state.deref().get('connected')) {
-            let client = util.getSession(util.getFileType(document));
+
+        if (util.getConnectedState()) {


My knowledge of typescript is limited, but I'd suggest something like:

// in-ns util function with-connection(f, ...) { function (args) { if (util.getConnectedState() { return f(args); } else { return "Please connect to a REPL to retrieve information." } }

Also, I'd recommend decoupling this from the hover functionality itself.
It seems as provideHover "bottoms out" in
return new vscode.Hover(someString)
So I'd suggest:

calculateHoverString(document, position) { const text = util.getWordAtPosition(document, position); const ns = util.getNamespace(document); const client = util.getSession(util.getFileType(document)); if(client) { await util.loadFileIfNamespaceNotExist(document); let res = await client.info(ns, text); // I really don't not why result.arglists-str does // not work, but it leads to an compiler error. if (res.ns && res.doc) { return this.formatDocString(res.ns + "/" + res.name, res["arglists-str"] || [], res.doc) } if (res.ns) { return this.formatDocString(res.ns + "/" + res.name, res["arglists-str"] || [], "No documentation available"); } else { return this.formatDocString(text, "", "No information available"); } } return this.formatDocString(text, "", "No information available"); } async provideHover(document, position, _) { const message = util.withConnection(calculateHoverString(document, position); return new score.Hover(message); }

If you go in this direction, you could potentially unit-test calculateHoverString

slipset · 2019-10-06T13:25:55Z

src/utilities.ts

@@ -190,6 +190,29 @@ function getWordAtPosition(document, position) {
    return text;
 }

+async function loadFileIfNamespaceNotExist(doc) {
+
+    if (getConnectedState()) {


I'd suggest writing this in a guard-clause fashion, and also to possibly throw, since I would assume that it's an error to try loading a ns that we cannot load. Also, by throwing exceptions, we can inform the caller of why we didn't succeed:

async function loadFileIfNamespaceDoesNotExist(doc) { if (!getConnectedState()) { throw new Error("Not connected"); } const document = getDocument(doc); if (!document) { throw new Error("Couldn't find " + doc) } const client = getSession(getFileType(document)); if (!client) { throw new Error("No client available for " + document); } const ns = getNamespace(document); const nsList = await client.listNamespaces([]); if (nsList['ns-list'] && nsList['ns-list'].includes(ns)) { return; } const name = getFileName(document); const dir = document.hasOwnProperty('fileName') ? path.dirname(document.fileName) : ''; await client.loadFile(document.getText(), name, dir); }

slipset · 2019-10-06T13:29:46Z

src/utilities.ts

+    return state.deref().get('launching');
+}
+
+function setLaunchingState(value) {


This is

function setLaunchingState(value) { vscode.commands.executeCommand("setContext", "calva:launching", Boolean(value)); state.cursor.set('launching', value); }

@slipset Good point!

PEZ · 2019-10-06T15:16:05Z

Many thanks for chipping in, @slipset !

…' of github.com:cfehse/calva into fix/general-stability-fixes-arglist-in-completion-hover

cfehse · 2019-10-06T16:30:40Z

@PEZ @kstehn

I change the mechanism to enable the info data to be retrieved (autoload namespace thing): if the namespace does not exists in the REPL it is now simply created by evaluating a form like (ns <namespace>). That is enough to get info data from the REPL for everything outside the file. If the user wants to have all information for the file, the file must be loaded. This avoids an option and does not interfere with the workflow of the users.

However, in a .cljc file, it only seems to do it for the clj repl. If I toggle the file to be backed by the cljs REPL, then I need to manually load the file. (We can add this as a separate issue if it looks tricky to solve now.)

I choose the corresponding REPL for the document using util.getSession(util.getFileType(document)) so the info call goes to the right REPL. If there is an issue I think it is not related to this PR.

Can I haz deps.edn added too?

Yes you can. gg

Maybe **/.{clj,edn} would work, even...

I would not do this because I find it more robust to surely have any supported project file in the workspace to activate the extension.

@slipset

I incorporated some of your suggestions - thanks for that. Refactor out some code into a new helper unit and decouple some of the hover and completion code I would like to be done in another PR. This is large enough.

…tion-hover

… into dev

…calva into dev

…o dev

PEZ · 2019-10-10T16:29:59Z

src/utilities.ts

@@ -190,24 +190,19 @@ function getWordAtPosition(document, position) {
    return text;
 }

-async function loadFileIfNamespaceNotExist(doc) {
+async function CreateNamespaceFromDocumentIfNotExists(doc) {


We name functions using camelCase, in our undocumented code convention. 😄

Good point! Changed...

cf and others added 4 commits October 4, 2019 17:01

Various stability improvments and arglist support.

92d4b25

See PR description for details.

Build changed strip-oef URI.

a21f1ea

Fix broken link to Wiki

9bd8c27

The link to About-Calva-Jack-in should be (I believe!) to Connect-Calva-to-Your-Project

Merge pull request BetterThanTomorrow#376 from paulbutcher/patch-1

132097a

Fix broken link to Wiki

PEZ reviewed Oct 5, 2019

View reviewed changes

src/providers/hover.ts Show resolved Hide resolved

kstehn reviewed Oct 5, 2019

View reviewed changes

src/nrepl/jack-in.ts Outdated Show resolved Hide resolved

kstehn reviewed Oct 5, 2019

View reviewed changes

Merge branch 'dev'

38d6286

slipset reviewed Oct 6, 2019

View reviewed changes

cf added 2 commits October 6, 2019 17:07

Corrected spelling of disconnect.

f797400

Added "workspaceContains:**/deps.edn" to activation events.

bd8cb4e

cf added 5 commits October 6, 2019 17:16

Corrected calvaJackInOrConnect() function as adived.

6f2ce78

Simplefied code as adviced.

fc915f3

Reimplemented mechanism to create namespace if info REPL call.

4eff613

Removed unused function and renamed used function.

bdaf3b2

Merge branch 'fix/general-stability-fixes-arglist-in-completion-hover…

99548a4

…' of github.com:cfehse/calva into fix/general-stability-fixes-arglist-in-completion-hover

cf and others added 13 commits October 6, 2019 18:35

Merge branch 'dev' into fix/general-stability-fixes-arglist-in-comple…

49bd4d0

…tion-hover

Merge HEAD, branch 'dev' of https://github.com/BetterThanTomorrow/calva…

77c9468

… into dev

Merged dev from upstream

d5bd219

Changed comment to reflect changes.

d75e491

Merged dev from upstream.

57c8fb6

Merge branch 'dev'

808866a

Merge branch 'dev'

91af0ae

Prpare for v2.0.47

9a9d96f

Merge HEAD, branch 'master' of https://github.com/BetterThanTomorrow/…

38fc30b

…calva into dev

Merge branch 'dev' of https://github.com/BetterThanTomorrow/calva int…

67f5f8f

…o dev

Merged dev from upstream.

8541fe0

Merge branch 'dev' of https://github.com/BetterThanTomorrow/calva int…

3c4bc6a

…o dev

MErged dev from upstream

acddd11

PEZ reviewed Oct 10, 2019

View reviewed changes

Renamed function to camelCase name.

ff795b5

PEZ merged commit 058dd2b into BetterThanTomorrow:dev Oct 10, 2019

cfehse deleted the fix/general-stability-fixes-arglist-in-completion-hover branch October 12, 2019 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General stability fixes and arglist support in completion hover #375

General stability fixes and arglist support in completion hover #375

cfehse commented Oct 4, 2019 •

edited

Loading

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

kstehn Oct 5, 2019

cfehse Oct 6, 2019

kstehn Oct 5, 2019 •

edited

Loading

cfehse Oct 6, 2019

slipset Oct 6, 2019

slipset Oct 6, 2019

slipset Oct 6, 2019

slipset Oct 6, 2019 •

edited

Loading

slipset Oct 6, 2019

cfehse Oct 6, 2019

PEZ commented Oct 6, 2019

cfehse commented Oct 6, 2019

PEZ Oct 10, 2019

cfehse Oct 10, 2019

General stability fixes and arglist support in completion hover #375

General stability fixes and arglist support in completion hover #375

Conversation

cfehse commented Oct 4, 2019 • edited Loading

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

cfehse commented Oct 5, 2019

PEZ commented Oct 5, 2019

PEZ commented Oct 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstehn Oct 5, 2019 • 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

slipset Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PEZ commented Oct 6, 2019

cfehse commented Oct 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfehse commented Oct 4, 2019 •

edited

Loading

kstehn Oct 5, 2019 •

edited

Loading

slipset Oct 6, 2019 •

edited

Loading