all: implement workspace/reference v2 #48

slimsag · 2016-12-06T03:27:49Z

According to the new specification at sourcegraph/language-server-protocol#8

Please do not merge until that PR is.

keegancsmith

Please do not merge until that PR is.

How easy would it be to update? Assuming we don't get the other PR merged just yet, if its easy to update lets merge in and get some stuff in prod :)

keegancsmith · 2016-12-07T11:33:37Z

langserver/workspace_refs.go

+				// Non-standard fields.
+				"dir":    defPkg.Dir, // TODO: emit standard "file" instead?
+				"pkg":    defPkg.ImportPath,
+				"vendor": IsVendorDir(defPkg.Dir),


shouldn't these fields live in meta?

Edit: forgot to remove this comment, later commits fixed this

keegancsmith · 2016-12-07T11:37:57Z

langserver/workspace_refs.go

@@ -217,7 +217,6 @@ func (h *LangHandler) externalRefsFromPkg(ctx context.Context, bctx *build.Conte
 				ContainerName: defContainerName,
 				PackageName:   defPkg.ImportPath,
 				Meta: map[string]interface{}{
-					"dir":    defPkg.Dir, // TODO: emit standard "file" instead?


surprised you didn't need to update tests with this commit?

Edit: I see you updated in a later commit

according to the new specification at sourcegraph/language-server-protocol#8 - langserver: remove 'dir' from workspace/reference - It's not useful to us, and would have to be translated by the build server into a repo-relative path anyway. - simplify goRangesToLSPLocations implementation

slimsag · 2016-12-08T21:03:29Z

I've updated the workspace/xreference implementation to reflect the latest specification.
I've also added a textDocument/xdefinition implementation according to the spec.
I've also squashed this down into just a single commit for workspace/xreferences and textDocument/xdefinition.

The xreference impl has not changed much since @keegancsmith reviewed it, but the second commit / xdefinition implementation is new so I'll need to get a review on that. @keegancsmith I will ask someone else to review because I want to land this into master today, but please leave comments etc here and I will follow up tomorrow morning.

This is useful for integration tests in the Sourcegraph core codebase.

beyang · 2016-12-08T21:42:14Z

langserver/definition.go

+		l.Location.Range.End = l.Location.Range.Start
+
+		// Determine metadata information for the node.
+		//rootFile := pkg.Files[0] // TODO: is this correct?


remove commented-out code

(hint: it was not correct :) )

beyang · 2016-12-08T21:44:31Z

langserver/definition.go

+
+		// Determine metadata information for the node.
+		//rootFile := pkg.Files[0] // TODO: is this correct?
+		def, err := refs.DefInfo(pkg.Pkg, &pkg.Info, pathEnclosingInterval, node.Pos())


Since def is only used within the following if-else block, a clearer way to write this would be:

if def, err := refs.DefInfo(pkg.Pkg, &pkg.Info, pathEnclosingInterval, node.Pos()); err == nil { } else }

beyang · 2016-12-08T21:51:40Z

langserver/workspace_refs.go

@@ -189,10 +180,6 @@ func (h *LangHandler) externalRefsFromPkg(ctx context.Context, bctx *build.Conte
 	}()
 	span.SetTag("pkg", pkg)

-	pkgInWorkspace := func(path string) bool {


If we now emit references to both internal and external definitions, you should change the name and docstring of this function.

beyang · 2016-12-08T21:54:21Z

pkg/lspext/lspext.go

+	Symbol    SymbolDescriptor `json:"symbol"`
+}
+
+type SymbolDescriptor struct {


Please add a docstring for this type and each of the fields.

beyang · 2016-12-08T21:54:36Z

pkg/lspext/lspext.go

+// ReferenceInformation is the array response type for the `workspace/xreferences` extension
+//
+// See: https://github.com/sourcegraph/language-server-protocol/blob/master/extension-workspace-reference.md
+//
 type ReferenceInformation struct {


Please add a docstring for each of these fields.

beyang · 2016-12-08T21:56:14Z

pkg/lspext/lspext.go

+	File          string                 `json:"file,omitempty"`
+	ContainerName string                 `json:"containerName,omitempty"`
+	Vendor        bool                   `json:"vendor,omitempty"`
+	Meta          map[string]interface{} `json:"meta,omitempty"`


If SymbolDescriptor itself is "metadata", having a Meta field inside it seems redundant. What about something like Attributes?

That would be a spec change, too. But okay.

- Remove some useless/commented code. - Write an if-else block in a clearer way. - Cleanup documentation + wording (external -> workspace) - Add detailed docstrings for all lspext types/fields. - Rename `SymbolDescriptor.Meta` to `SymbolDescriptor.Attributes`

…parent` meta fields This give us less ambiguity.

slimsag · 2016-12-09T01:42:29Z

Also see: sourcegraph/sourcegraph#2673

- Fix issue with `name` and `attr_parent` being reversed when `attr_parent` is present. - Remove commented-out code that was missed.

slimsag assigned keegancsmith Dec 6, 2016

keegancsmith approved these changes Dec 7, 2016

View reviewed changes

slimsag force-pushed the sg/workspace-reference-v2 branch from 6610dcc to 45b2f17 Compare December 8, 2016 20:58

slimsag added 2 commits December 8, 2016 12:59

implement textDocument/xdefinition according to the spec

364a072

slimsag force-pushed the sg/workspace-reference-v2 branch from 45b2f17 to 364a072 Compare December 8, 2016 20:59

enable SymbolDescriptor stringer to be accessed outside of package

b49c169

This is useful for integration tests in the Sourcegraph core codebase.

beyang reviewed Dec 8, 2016

View reviewed changes

slimsag added 2 commits December 8, 2016 16:21

langserver: replace containerName with separate packageName and `…

de5b98d

…parent` meta fields This give us less ambiguity.

slimsag merged commit 526e8a1 into master Dec 9, 2016

slimsag deleted the sg/workspace-reference-v2 branch December 9, 2016 01:42

slimsag added a commit that referenced this pull request Dec 9, 2016

langserver: followup for PR #48

bf3adf4

- Fix issue with `name` and `attr_parent` being reversed when `attr_parent` is present. - Remove commented-out code that was missed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: implement workspace/reference v2 #48

all: implement workspace/reference v2 #48

slimsag commented Dec 6, 2016 •

edited

keegancsmith left a comment

keegancsmith Dec 7, 2016 •

edited

keegancsmith Dec 7, 2016

slimsag commented Dec 8, 2016

beyang Dec 8, 2016

slimsag Dec 8, 2016

beyang Dec 8, 2016

beyang Dec 8, 2016

slimsag Dec 9, 2016

beyang Dec 8, 2016

beyang Dec 8, 2016

beyang Dec 8, 2016

slimsag Dec 8, 2016

slimsag commented Dec 9, 2016

all: implement workspace/reference v2 #48

all: implement workspace/reference v2 #48

Conversation

slimsag commented Dec 6, 2016 • edited

keegancsmith left a comment

Choose a reason for hiding this comment

keegancsmith Dec 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimsag commented Dec 8, 2016

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

slimsag commented Dec 9, 2016

slimsag commented Dec 6, 2016 •

edited

keegancsmith Dec 7, 2016 •

edited