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

Comments

@inliquid
Copy link

@inliquid inliquid commented Aug 11, 2019

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 label Aug 11, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 12, 2019

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").

@stamblerre stamblerre closed this Aug 12, 2019
@inliquid

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 12, 2019

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.

@kentquirk

This comment has been minimized.

Copy link

@kentquirk kentquirk commented Aug 30, 2019

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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Aug 30, 2019

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

This comment has been minimized.

Copy link
Author

@inliquid 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

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Aug 30, 2019

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

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Aug 30, 2019

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

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Aug 30, 2019

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

This comment has been minimized.

Copy link

@alexanderbez alexanderbez commented Aug 31, 2019

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

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Aug 31, 2019

Yes, that's #32049.

@gopherbot gopherbot added the Tools label Sep 12, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik 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

This comment has been minimized.

Copy link
Author

@inliquid 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?

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik 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.

@heschik heschik closed this Nov 12, 2019
@inliquid

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Nov 13, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.