Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: add "if err != nil" snippet completion for net/http's HandlerFunc #48487

Open
marwan-at-work opened this issue Sep 20, 2021 · 8 comments
Open

x/tools/gopls: add "if err != nil" snippet completion for net/http's HandlerFunc #48487

marwan-at-work opened this issue Sep 20, 2021 · 8 comments
Labels
gopls Issues related to the Go language server, gopls. Thinking Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/backlog

Comments

@marwan-at-work
Copy link
Contributor

The net/http HandlerFunc is a very common function that Go developers write:

func handler(w http.ResponseWriter, r *http.Request)

Since the function doesn't return any errors, most users will have to handle the error in the following way:

func handler(w http.ResponseWriter, r *http.Request) {
  results, err := doSomething()
  if err != nil {
    http.Error(w, err.Error(), http.StatusInternalServerError)
    return
  }
  // use results
}

This can be quite tedious to write and people (aka me) often forget to add the "return" statement which ends up causing the net/http library to spit out errors about multiple status codes being written to the same response.

In production applications, Go developers typically structure their http servers in a way that will minimize the amount they have to do the HTTP error handling above. But they most likely still have to do it once per handler, or sometimes they want to prototype a server quickly without having to think about how to nicely structure their code to DRY up the error handling logic.

Therefore, as a continuation of improvements to our snippet completions mentioned in 43310, I'd love to add a completion that will simply offer to write the above error handling snippet for you. It should also put placeholders on the error and status code arguments while picking sane defaults such as err.Error() and http.StatusInternalServerError.

Since the code structure for snippet completions is already in place, adding this use case should be a minimal change. I've included a CL to show the type of change needed, which can be reviewed/merged if this issue is accepted.

Thanks!
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Sep 20, 2021
@gopherbot gopherbot added this to the Unreleased milestone Sep 20, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/351030 mentions this issue: internal/lsp: add snippet completion for errors inside http.HandlerFunc

@suzmue suzmue modified the milestones: Unreleased, gopls/on-deck Sep 20, 2021
@stamblerre
Copy link
Contributor

Thanks for filing this and for mailing a CL! I've never really used net/http, and I'm wondering if there is some documentation about this being the canonical or industry-standard way of handling errors. It would be nice to have that linked in the code, especially in case people disagree.

@stamblerre
Copy link
Contributor

/cc @golang/pkgsite team, since they have more experience with net/http

@stamblerre stamblerre added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. Thinking and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 28, 2021
@jba
Copy link
Contributor

jba commented Sep 29, 2021

That snippet is the sort of code you often see, but it is clumsy and error-prone (forgetting the return is bad). In pkgsite we define a wrapper so handlers can return errors. I imagine this is common too.

I don't think there's one canonical approach to handling errors. I also don't know what your criteria are for adding a snippet. The code given is certainly correct but will having the snippet discourage people from writing a wrapper, which is a cleaner approach?

@stamblerre
Copy link
Contributor

Thanks for insight, @jba! We have been fairly hesitant about adding snippets, since it might seem like we're prescribing a "correct" or "Go team approved" way of doing things.

@marwan-at-work: How would you feel about an if err != nil { <> } snippet, where the user's cursor would be placed at the <>?

@marwan-at-work
Copy link
Contributor Author

We have been fairly hesitant about adding snippets, since it might seem like we're prescribing a "correct" or "Go team approved" way of doing things.

That's totally fair :)

How would you feel about an if err != nil { <> } snippet, where the user's cursor would be placed at the <>?

I believe this already exists for editors like VSCode on the client side (if you type iferr and press enter). Another potential snippet is the following:

if err != nil { <> \n return }

If that looks good, I'd be happy to amend the CL.

But to be honest, I think the best solution here is to make gopls extensible so that people can write their own snippets that is specific to their usecases. Is there an issue open to follow or discuss this alternative?

@stamblerre thank you!

@stamblerre
Copy link
Contributor

If that looks good, I'd be happy to amend the CL.

Yeah, that sounds good to me! We've been discussing moving the client side snippets to gopls anyway, and I like this because we'll still be offering that snippet, but just in the right contexts.

But to be honest, I think the best solution here is to make gopls extensible so that people can write their own snippets that is specific to their usecases. Is there an issue open to follow or discuss this alternative?

Does something like https://code.visualstudio.com/docs/editor/userdefinedsnippets#_create-your-own-snippets work for you? I know it is client-side, but I still think that's a little easier than integrating custom snippets with gopls.

@marwan-at-work
Copy link
Contributor Author

We've been discussing moving the client side snippets to gopls anyway, and I like this because we'll still be offering that snippet, but just in the right contexts.

Awesome. I just updated the CL 👍🏼

Does something like https://code.visualstudio.com/docs/editor/userdefinedsnippets#_create-your-own-snippets work for you? I know it is client-side,

Unfortunately that would not work because the snippet will need to analyze the context it is in (for example checking the enclosing function signature, etc).

but I still think that's a little easier than integrating custom snippets with gopls.

To be honest, custom snippets is not the only thing that could benefit from extensibility. Basically any LSP feature can be extended if a user wanted the LSP feature to do something that may not make sense in gopls but is helpful to that user.

@marwan-at-work marwan-at-work mentioned this issue Oct 16, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Thinking Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/backlog
Development

No branches or pull requests
5 participants
@stamblerre @suzmue @gopherbot @marwan-at-work @jba