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.

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

x/tools/gopls: format feature doesn't follow goimports #33587

Closed
inliquid opened this issue Aug 11, 2019 · 15 comments
Closed

x/tools/gopls: format feature doesn't follow goimports #33587

inliquid opened this issue Aug 11, 2019 · 15 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@inliquid
Copy link

For example, here is what imports block looks like in VS Code after pressing Alt-Shift-F (formatting) with enabled gopls:
изображение

And here is what it looks like after pressing Ctrl-S (forcefully save file):
изображение

For p.2, these options are enabled for gopls:

        "editor.formatOnSave": true,
        "editor.codeActionsOnSave": {
            "source.organizeImports": true
        },

I'd like to have an ability to configure gopls to follow goimports style when formatting the file or to have this behavior as default.

Related:
If I remove fmt dependency from code, but leave it in imports section, formatting will not remove fmt:
изображение

@gopherbot gopherbot added this to the Unreleased milestone Aug 11, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Aug 11, 2019
@stamblerre
Copy link
Contributor

This is intentional. In gopls, formatting is separate from organizing imports (hence the two different settings). Formatting a file just means that we will run gofmt on it, whereas organizing imports means that we will run the equivalent of goimports. Our intention is to decouple the two behaviors.

If you would like to manually organize imports, you can run the organize imports source action (Ctrl + Shift + P -> Source Action -> Organize Imports). It will only be available if your imports are incomplete. Furthermore, if you see a diagnostic indicating that you are using a package that has not been imported, we will offer you a quick fix suggestion so that you can automatically import it (just hover over the place with the error and select "quick fix").

@inliquid
Copy link
Author

Maybe let others say their word instead of just closing all my issues? I believe you are very wrong with that approach. At least it's non-professional.

There was a setting both in Sourcegraph's go-langserver and in bingo. There is setting in VS Code which allows to select gofmt or goimports for formatting. Most of the gophers I know use goimports-like for formatting in VS Code and Goland (last one is on a cosmic distance in all that, which makes use of gopls ridiculous).

@stamblerre
Copy link
Contributor

I apologize for closing your issue prematurely, but you are welcome to continue commenting if you feel I have made a mistake. Triaging a large number of issues is a difficult task, and I typically prefer to close an issue if I feel I have answered a question.

As I explained in my comment, gopls uses both gofmt and goimports. The LSP specification separates out the behaviors of formatting a file and organizing the imports in a file, so we have chosen to split these behaviors along those lines. This explains the differences in the behavior of formatting and saving a file.

I have re-opened this issue so that we can continue the discussion. gopls is significantly different from previous Go tooling, so our goals here are not necessarily to make decisions that are in line with past behaviors.

@stamblerre stamblerre reopened this Aug 12, 2019
@stamblerre stamblerre added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 13, 2019
@kentquirk
Copy link

I very much like the fact that with goreturns (which is what I use) my import statements are always made organized and correct whenever I save the file.

Example: I'm debugging and I add a fmt.Println() statement temporarily; goreturns ensures that fmt is now in my imports list. I debug the code and remove that statement; again goimports corrects my imports list. Adding two manual steps to that process doesn't seem to benefit my workflow. What am I missing?

@ianthehat
Copy link

There are no extra manual steps.
With the suggested settings, we both format and fix imports on save.
The difference is that we consider them separate actions and allow you to do them without saving and on their own.
This bug is asking for a way to make is so that triggering formatting will also fix your imports.

@inliquid
Copy link
Author

inliquid commented Aug 30, 2019

This bug is asking to make formatting provider configurable, as it was for years already with vscode-go extension (and in fact with any other major modern editor)!
изображение
I don't want formatting on save as my files are being saved automatically, I want goimports-like formatting to be applied when I press Alt-Shift-F == format file action. As it happens already when I turn off gopls.

@inliquid
Copy link
Author

This setting was also available in both bingo and go-langserver.

@ianthehat
Copy link

I don't think the formatting provider has never been configurable, the save hook was.

Most people chose a save hook that did both formatting and something else, of which the most common by far was goimports which both formats and fixes imports. We chose to correctly split that functionality at the language server level so that editor integrations could choose how to combine the bits into a good user experience.

Alt-Shift-F is a key binding that is bound to formatting (without saving) by default, which in language server implementations triggers the formatting calls. It would be technically wrong for a language server implementation to do more than formatting in a format call.

If you want a function in your editor that triggers both formatting and import fixing, then you need to ask for that feature in your editor plugin, and you can bind it to any key you like, that is not a gopls feature.

@inliquid
Copy link
Author

Alt-Shift-F is a key binding that is bound to formatting (without saving) by default, which in language server implementations triggers the formatting calls. It would be technically wrong for a language server implementation to do more than formatting in a format call.

That's wrong. See screenshot above, how it's done by VS Code without gopls. If gopls is a replacement for go tools, it must provide this option as well. Atm you deliver degraded user experience.

@alexanderbez
Copy link

Apologies if this has been asked, but is it configurable via gopls settings to set a --local flag like you could with goimports? Or is there another config that allows imports to be "grouped"?

@zikaeroh
Copy link
Contributor

Yes, that's #32049.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi commented Nov 12, 2019

Now that I've fixed #30843, I'm taking a look at this issue. I've read through it a couple times and I don't see anything left that's actionable. As discussed above, LSP defines what formatting means, and we don't want to cause problems for any LSP clients by violating their expectations. Ultimately editors have control over the UI, and it seems reasonable to me that VSCode could offer a shortcut that both formatted and organized imports.

@inliquid: Judging by the lack of traffic on this issue, this seems to be a problem specific to your workflow. That doesn't necessarily mean we shouldn't fix it, but it would help if you explained why it's important to you rather than just saying that gopls provides a "degraded user experience". In particular, it's unclear to me why you can't simply save the file to trigger formatting and import organization.

@inliquid
Copy link
Author

inliquid commented Nov 12, 2019

@heschik I think it was explained pretty much clear.

PS: I'm a user of both Goland and VS Code. And last becomes less over time. Maybe that is where your "traffic" goes?

@heschi
Copy link
Contributor

heschi commented Nov 12, 2019

Okay. I can't figure out anything to do here, so I'm going to close the issue. If GoLand works well for you, then by all means stick with that.

@heschi heschi closed this as completed Nov 12, 2019
@inliquid
Copy link
Author

This is more an advice for others, who still trying to use VS Code with gopls. I will of course use what's better for me, like any other person. Most of the time people vote by their "legs", and do not want to spend time on stupid github issues.

johnboyes added a commit to agilepathway/label-checker that referenced this issue Jul 5, 2020
Imports are instead handled by the gopls language server:
golang/go#33587 (comment)
johnboyes added a commit to agilepathway/label-checker that referenced this issue Jul 5, 2020
Imports are instead handled by the gopls language server:
golang/go#33587 (comment)
johnboyes added a commit to agilepathway/label-checker that referenced this issue Jul 5, 2020
Imports are instead handled by the gopls language server:
golang/go#33587 (comment)
johnboyes added a commit to agilepathway/label-checker that referenced this issue Jul 5, 2020
* Enable Codespaces - add default Go files

Codespaces[1] allows us to use a remote vscode environment embedded in
the browser in GitHub[1]

This commit adds the pre-built container configuration[2] for Go[3].
We can customise this configuration as we wish in future commits.

[1] https://github.com/features/codespaces/
[2] https://docs.github.com/en/github/developing-online-with-codespaces/configuring-codespaces-for-your-project#using-a-pre-built-container-configuration
[3] https://github.com/microsoft/vscode-dev-containers/tree/master/containers/go

* Suppress/fix devcontainer Dockerfile warnings

These were hadolint warnings, and there were three of them, all on
.devcontainer/Dockerfile#L17:

DL3003 Use WORKDIR to switch to a directory

DL3008 Pin versions in apt get install.
Instead of `apt-get install <package>`
use `apt-get install <package>=<version>`

DL3015 Avoid additional packages by specifying `--no-install-recommends`

Suppressed the warnings apart from the DL3015 one, which was easy to
fix.

Not fixing the other warnings (for now anyway) as this Dockerfile is
code that has been lifted and shifted from the
`.devcontainer/Dockerfile` in:
https://github.com/microsoft/vscode-dev-containers/tree/master/containers/go

* Update module settings to "on" as we use modules

See:
https://dev.to/maelvls/why-is-go111module-everywhere-and-everything-about-go-modules-24k

* Add shellcheck vscode extension to devcontainer

https://www.shellcheck.net/
https://marketplace.visualstudio.com/items?itemName=timonwong.shellcheck

* Use vscode go language server on devcontainer

https://github.com/golang/vscode-go/blob/master/docs/gopls.md

* Stop installing go dependencies on devcontainer

There is no need to install them as we are using go modules - see:
microsoft/vscode-go#2836

* Set vscode editor go tabsize to correct size of 8

See:
microsoft/vscode-go#2479 (comment)

* Use golangci-lint for vscode devcontainer linting

https://github.com/golangci/golangci-lint

* No longer install Guru on devcontainer

It does not support Go modules:
https://github.com/golang/vscode-go/blob/master/docs/tools.md#guru

* Remove unnecessary gorename from devcontainer

Not needed when using go modules and gopls language server:
https://github.com/golang/vscode-go/blob/master/docs/tools.md#gorename

* Remove unnecessary godoctor from devcontainer

It is not needed when using go modules and the gopls language server:
https://github.com/golang/vscode-go/blob/master/docs/tools.md#godoctor

* Remove unnecessary goimports from devcontainer

Imports are instead handled by the gopls language server:
golang/go#33587 (comment)

* Remove unnecessary golint from devcontainer

Not needed as we are using golangci-lint

* Remove unnecessary gotests from devcontainer

gotests autogenerates table tests boilerplate code.  We don't need this
for this project.

* Remove unnecessary goplay module from devcontainer

We don't need this functionality:
https://github.com/haya14busa/goplay/

* Remove unnecessary gometalinter from devcontainer

Not needed as we are using golangci-lint for linting.

* Remove unnecessary impl module from devcontainer

It does not have support for go modules:
golang/go#37537

* Remove unnecessary fillstruct from devcontainer

fillstruct fills a struct literal with default values[1]
This functionality is now available in the gopls language server[2]

[1] https://github.com/davidrjenni/reftools/tree/master/cmd/fillstruct
[2] golang/go#37576 (comment)

* Add comment to explain why gopkgs is still needed

It is still needed even with gopls:
microsoft/vscode-go#3050 (comment)

* Fix installation of golangci-lint on devcontainer

Prior to this commit, vscode was saying "Analysis tools missing" and
prompting to install it.
Fix was to install golangci-lint via go get, as per:
golangci/golangci-lint#1037 (comment)

* Remove unnecessary gogetdoc from devcontainer

Similar functionality is available in the gopls language server:
fatih/vim-go#2808 (comment)

* Remove unnecessary revive linter from devcontainer

We are using golangci-lint for linting, so no need for revive.

* Remove unnecessary go-tools from devcontainer

go-tools primarily contains staticcheck, which we don't need as we are
using golangci-lint for linting.

* Output go version after building devcontainer

Doing this just to provide assurance that the container has started
successfully, after building.

* Move settings which are not container-specific

The guideline for`devcontainer.json` settings is that they should only
contain settings which _must_ be be changed in a container (e.g.
absolute paths)[1].

Moved the other settings to `.vscode/settings.json` so that they are
more visible, and easier to use when working locally (i.e. not in a
container).

[1] microsoft/vscode-remote-release#874 (comment)

* Up tests timeout as tests are slower on container

The tests seem to run much slower (c. 100 seconds compared to c. 50
seconds) when running on a remote container, compared to locally.
Will investigate to see if this is the same when running on Codespaces.

If the tests are taking 100 seconds we may need to create an issue to
speed them up.

* Add recommended vscode go settings

As per the recommendations at:
https://github.com/golang/tools/blob/master/gopls/doc/vscode.md

* Set dark colour theme for vscode in devcontainer

* Set devcontainer vscode to autosave after 500ms

Adding these settings to the devcontainer settings file rather than the
vscode settings file, as we want them to apply at the machine level.

https://code.visualstudio.com/docs/editor/codebasics#_save-auto-save
@golang golang locked and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

8 participants