Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Due to issue #599, use gogetdoc instead of godef #607

Closed
wants to merge 6 commits into from
Closed

Due to issue #599, use gogetdoc instead of godef #607

wants to merge 6 commits into from

Conversation

leaxoy
Copy link
Contributor

@leaxoy leaxoy commented Nov 8, 2016

Changes:
1 use gogetdoc instead of godef and godoc. Now can use only gogetdoc tool and only one step get both definition info and doc.
2 hover info, old one is doc before signature, now is signature before doc. Since other language or extension dose.
3 support builtin package that no definition info in gogetdoc properties.( PS:but can't jump to source.)

@zmb3
Copy link
Contributor

zmb3 commented Nov 9, 2016

@abarisain how does this compare to your changes?

@abarisain
Copy link
Contributor

My version is much more complicated because it retained compatibility with the old tools for users stuck on old Go versions (it was a wish of the maintainer of this plugin)

However I've got 0 time to work on it (really sorry about getting your hopes up), so if this works, I'd say go for it :) It looks like it does more than what I did

@ramya-rao-a
Copy link
Contributor

We do want to continue supporting Go 1.5 Does anybody know the usage matrix between Go 1.5, 1.6 and 1.7 ?

@zmb3
Copy link
Contributor

zmb3 commented Nov 9, 2016

A solution that I've suggested before for supporting older versions of Go is just to bundle or download a pre-built version of the gogetdoc tool. The tool only needs Go 1.6 to build, but will run just fine on code bases and developer environments that are < Go 1.6.

I would be happy to publish github releases for the major OS/arch combos if that helps.

@abarisain
Copy link
Contributor

I didn't know that was possible! It would be clean.

Supporting two tools version isn't very had either. The classes are well made, you just have to swap implementations.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 9, 2016

Right now users do a "go get" which would download and build and install. How would they be downloading the pre-built tool?

@zmb3
Copy link
Contributor

zmb3 commented Nov 9, 2016

@ramya-rao-a right. What I'm suggesting is a toggle whereby the extension checks the version of Go in the user's environment. If Go 1.6+, it should perform the go get like normal, otherwise it should download a pre-built binary.

Alternatively, do what @abarisain did in his fork - check the version of Go and if less than 1.6 fallback to the old godoc + godef approach (which has limitations but works with the typical go get approach).

Either way, if you want to support older versions of Go then the extension needs to do a check.

@ramya-rao-a
Copy link
Contributor

@zmb3 Oh, I got the need to check for Go version part :) I was curious about downloading the pre-built tool. For example, would we be downloading an exe in case of Windows from which ever location you publish them in?

@zmb3
Copy link
Contributor

zmb3 commented Nov 9, 2016

That's an option, yes. I can definitely publish releases on github with builds for Windows, mac OS, and Linux.

I could see the argument that you then depend on me to not pull the release, and while I plan to support the tool for the foreseeable future and want to see its use in as many editors as possible, you are certainly free to make your own builds and host them on a Microsoft server somewhere or even bundle the binary in the extension download.

@leaxoy
Copy link
Contributor Author

leaxoy commented Nov 9, 2016

I admit in some case I'm extreme.
If necessary, support go version less 1.6 is a good idea. But if 1.8 released, should we drop support or continue support.
But it is complicated to integrate similar function tool in it. Is there any data about usage of go with different version(1.5, 1.6, 1.7)

@leaxoy
Copy link
Contributor Author

leaxoy commented Nov 10, 2016





@zmb3
Copy link
Contributor

zmb3 commented Nov 10, 2016

Looks great!

@leaxoy
Copy link
Contributor Author

leaxoy commented Nov 11, 2016

Is someone give suggestion on whether drop 1.5 support. But keep gometaliner as a more linter tool.

@ramya-rao-a
Copy link
Contributor

@leaxoy, sorry I didn't understand your last comment.
We wont be dropping Go 1.5 support without knowing some usage numbers.

lines = lines.filter(line => line.length !== 0);
if (lines.length > 10) lines[9] = '...';
let lines = definitionInfo.docInfo.decl.split('\n')
.filter(line => !line.startsWith('\t//') && line !== '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly are we trying to achieve here : line.startsWith('\t//') ?

Copy link
Contributor Author

@leaxoy leaxoy Nov 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment and blank line from type definition.
for example:

type Response struct {
    Status     string // e.g. "200 OK"
    StatusCode int    // e.g. 200
    Proto      string // e.g. "HTTP/1.0"
    ProtoMajor int    // e.g. 1
    ProtoMinor int    // e.g. 0

    // Header maps header keys to values. If the response had multiple
    // headers with the same key, they may be concatenated, with comma
    // delimiters.  (Section 4.2 of RFC 2616 requires that multiple headers
    // be semantically equivalent to a comma-delimited sequence.) Values
    // duplicated by other fields in this struct (e.g., ContentLength) are
    // omitted from Header.
    //
    // Keys in the map are canonicalized (see CanonicalHeaderKey).
    Header Header

    // Body represents the response body.
    //
    // The http Client and Transport guarantee that Body is always
    // non-nil, even on responses without a body or responses with
    // a zero-length body. It is the caller's responsibility to
    // close Body. The default HTTP client's Transport does not
    // attempt to reuse HTTP/1.0 or HTTP/1.1 TCP connections
    // ("keep-alive") unless the Body is read to completion and is
    // closed.
    //
    // The Body is automatically dechunked if the server replied
    // with a "chunked" Transfer-Encoding.
    Body io.ReadCloser

    // ContentLength records the length of the associated content. The
    // value -1 indicates that the length is unknown. Unless Request.Method
    // is "HEAD", values >= 0 indicate that the given number of bytes may
    // be read from Body.
    ContentLength int64

    // Contains transfer encodings from outer-most to inner-most. Value is
    // nil, means that "identity" encoding is used.
    TransferEncoding []string

    // Close records whether the header directed that the connection be
    // closed after reading Body. The value is advice for clients: neither
    // ReadResponse nor Response.Write ever closes a connection.
    Close bool

    // Uncompressed reports whether the response was sent compressed but
    // was decompressed by the http package. When true, reading from
    // Body yields the uncompressed content instead of the compressed
    // content actually set from the server, ContentLength is set to -1,
    // and the "Content-Length" and "Content-Encoding" fields are deleted
    // from the responseHeader. To get the original response from
    // the server, set Transport.DisableCompression to true.
    Uncompressed bool

    // Trailer maps trailer keys to values in the same
    // format as Header.
    //
    // The Trailer initially contains only nil values, one for
    // each key specified in the server's "Trailer" header
    // value. Those values are not added to Header.
    //
    // Trailer must not be accessed concurrently with Read calls
    // on the Body.
    //
    // After Body.Read has returned io.EOF, Trailer will contain
    // any trailer values sent by the server.
    Trailer Header

    // Request is the request that was sent to obtain this Response.
    // Request's Body is nil (having already been consumed).
    // This is only populated for Client requests.
    Request *Request

    // TLS contains information about the TLS connection on which the
    // response was received. It is nil for unencrypted responses.
    // The pointer is shared between responses and should not be
    // modified.
    TLS *tls.ConnectionState
}

to:

type Response struct {
    Status     string // e.g. "200 OK"
    StatusCode int    // e.g. 200
    Proto      string // e.g. "HTTP/1.0"
    ProtoMajor int    // e.g. 1
    ProtoMinor int    // e.g. 0
        Header Header
        Body io.ReadCloser
        ....
}

.... means rest fields. ignore the bad indent.

I thank keep a clear view in tooltip signature section and leave full doc at the tooltip doc section is a good idea.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 15, 2016

@leaxoy I have a PR out to your repo that does the below

  • A setting where user can choose godoc (old way of doing things godef + godoc) vs gogetdoc
  • If User is using Go 1.5, then we use the old way regardless of what is set
  • The setting will default to the old way. There are some 3 or 4 issues logged by users which using gogetdoc will solve. We will reach out to those users and ask them to change the setting to use gogetdoc and give feedback
  • Some refactoring of the interfaces

We can change the setting to default to gogetdoc next month or so.

Once you merge my PR to your repo, the tests in travis should start passing and then I can merge your PR :)

@abarisain
Copy link
Contributor

Impressive work :)

@ramya-rao-a
Copy link
Contributor

@leaxoy I wanted to release an update to the Go extension this week and was hoping to include your changes to use gogetdoc. Since I didn't hear back on the PR for the changes I made to your code, I created a separate PR #622 which has all your changes + mine.

Do take a look at #622

@ramya-rao-a
Copy link
Contributor

Closing this in favor of #622

@leaxoy @abarisain @zmb3 Do take a look at #622 if you have some time.
I'll merge and release an update on Monday

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants