Revive autocomplete in IDE #649 by JamesXNelson · Pull Request #682 · deephaven/deephaven-core

JamesXNelson · 2021-05-27T23:52:46Z

No description provided.

grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java

open-api/lang-parser/src/main/java/io/deephaven/lang/parse/LspTools.java

open-api/lang-tools/src/main/java/io/deephaven/lang/completion/results/CompleteInvocation.java

JamesXNelson · 2021-06-11T00:25:46Z

@niloc132 (finally) done updates. I know it wasn't much, just way too many install questions all day to make decent progress.

JamesXNelson · 2021-06-11T00:36:07Z

Also, the commit for newTable completion is JamesXNelson@1b2b811 but I think that one will be more likely to land on devin's plate, so 🤷

grpc-api/src/main/java/io/deephaven/grpc_api/console/ConsoleServiceGrpcImpl.java

niloc132 · 2021-06-11T19:04:27Z

open-api/lang-parser/lang-parser.gradle

    compile project(':open-api-shared-ide')
    compile project(':open-api-shared-fu')
+    compile project(':IO')
+    compile project(':proto:proto-backplane-grpc')


not an "issue", just a remark:

in most other parts of the repo, we've tried to keep proto contained to grpc-api, so that the semantics of how you make calls doesn't creep into other aspects of the codebase - provided nothing else (like db) depends on lang-parser I don't think this dep is a problem, but just pointing out the decision that was made.

ditto dagger2, we're keeping that in grpc-api, and just encouraging other code to use patterns that make it easy for code to have dependencies injected, but not require consumers of the codebase to use our di

ah whoops, db does_ now depend on open-api-lang-parser... may need to revisit this soon if not right away, make sure this is unavoidable

I had a diamond problem between the parser and the completer. The parser operates on "lsp types", so I used the grpc generated types. The completer needs objects like Table. The completer also needs the parser.

Advice on what to do here? If / when Table and TableDefinition (and VariableProvider) can be moved out of db, then I could resolve this, but, as is, unless I want to have three copies of the same objects in slightly different formats spread out across all the modules, I dunno how else to fix.

Well, the completer mostly operates on the lsp types, but I put utilities into the AST nodes to make them able to understand / translate between AST and LSP. I might be able to extract them all into static util methods; annoything, but probably not impossible. I'll take a quick spin at it.

oh man, there's lots and lots and lots of references :-/

I think we should merge it like this, and just open a ticket to refactor these dependencies once the Table interface is split up, and things are ....less likely to suck in the whole world. That said... I built everything using the grpc-backplane types, and found that, of the three versions of the same object, they were the best to work with (and seemed appropriate given it is all server side).

If the goal is to avoid having DB suck them in (why? I don't understand.... the term backplane doesn't mean a lot to me)... then, I suppose it'll be possible to come up w/ an interface and DI to get my parser into the script session (nate had agreed it was the best place to put it, and I don't know enough of the infrastructure to disagree).

Ok, pushed commit w/ a refactor. It's a little ugly w/ the type parameters, but the wider, shorter dependency tree is nicer.

niloc132 · 2021-06-22T17:49:47Z

open-api/lang-parser/src/main/java/io/deephaven/lang/parse/ParsedDocument.java

-        assert replaceRange.start.greaterOrEqual(startPos);
-        assert replaceRange.end.greaterOrEqual(endPos);
+        assert LspTools.greaterOrEqual(replaceRange.getStart(), startPos);
+        assert LspTools.greaterOrEqual(replaceRange.getEnd(), endPos);


jvm code should use Assert or Require if this is a concern

it's really not, just there for testing. Plus, it's possible for these types to someday live on a client, if that's ever something we'd want to bother w/.

niloc132 · 2021-06-22T17:50:37Z

open-api/lang-tools/src/main/java/io/deephaven/lang/completion/DelegatingCompletionHandler.java

+                Thread.currentThread().interrupt();
+                break;
+            } catch (ExecutionException e) {
+                // TODO: log this;


file issues for new TODOs, or link them in code - format is // TODO (repo#issuenum) message

I just stuck in a logger, and trace() log it, instead of throwing an exception and breaking things over autocomplete misbehaving.

niloc132

two minor thoughts, i'd only ask that the TODOs get updated across the PR before merge

niloc132 · 2021-06-22T17:53:59Z

web/client-api/src/main/java/io/deephaven/ide/shared/IdeSession.java

+        final JsPropertyMap<Object> posObj = pos.asPropertyMap();
+        final Position result = new Position();
+        result.setLine(posObj.getAny("line").asInt());
+        result.setCharacter(posObj.getAny("character").asInt());


i may revisit these later and make jsinterop "beans" for them, since asInt() has no power under prod builds, and will just write garbage - we will want/need something that even at runtime in prod gets typechecked for calls from non-typesafe code

luckily for us, these objects that come in from react will always be populated, so fine enough to leave like this for now.

JamesXNelson linked an issue May 27, 2021 that may be closed by this pull request

Get the minimal wiring to have an autocomplete service respond to client requests #649

Closed

JamesXNelson added the NoDocumentationNeeded label May 27, 2021

JamesXNelson force-pushed the jxn/autocomplete branch from 2aaca39 to fbffd40 Compare May 28, 2021 00:01

JamesXNelson assigned niloc132 Jun 3, 2021

niloc132 reviewed Jun 4, 2021

View reviewed changes

JamesXNelson force-pushed the jxn/autocomplete branch from fbffd40 to 0c549f0 Compare June 11, 2021 00:22

JamesXNelson force-pushed the jxn/autocomplete branch from 0c549f0 to 076d700 Compare June 11, 2021 00:32