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

Updates to vim section #1227

Merged
merged 4 commits into from Dec 30, 2019
Merged

Updates to vim section #1227

merged 4 commits into from Dec 30, 2019

Conversation

@ckipp01
Copy link
Member

ckipp01 commented Dec 27, 2019

Updates to vim documentation

  • Added in information about alternative ways to install
  • Changed out Doctor information for an embedded version
  • A new section about statusline integration
  • Fixed some typos
@@ -17,7 +16,8 @@ which will provide the most complete implementation of LSP and Metals specific h
## Installing Vim

The coc.nvim plugin requires either **Vim >= 8.1** or **Neovim >= 0.3.1**. Make
sure you have the correct version installed.
sure you have the correct version installed. While it works with both Vim and Neovim, the
experience is a bit smoother with Neovim.

This comment has been minimized.

Copy link
@ckipp01

ckipp01 Dec 27, 2019

Author Member

I originally wasn't going to mention anything about a preference for Neovim, but after spending some time with the new Vim 8.2, I feel pretty confident in saying that Neovim is a smoother experience. It's snappier with larger codebases for one, but another thing leading me to say this is that code actions seem sort of unusable at the moment with Vim 8.2 and coc.nvim (see neoclide/coc.nvim#1404)

@@ -133,7 +133,6 @@ object MetalsServerConfig {
case "coc.nvim" =>
base.copy(
statusBar = StatusBarConfig.showMessage,
isHttpEnabled = true,

This comment has been minimized.

Copy link
@ckipp01

ckipp01 Dec 27, 2019

Author Member

I've taken this out and there are also some other changes that I'll make to the configuration like isInputBoxEnabled being turned on and executeClientCommand also being on for coc-metals. However, I'm turning those on in the extension itself in order to allow someone to still bootstrap Metals themselves for coc.nvim and not use the extension. If they do that, then client commands and input boxes won't be implemented allowing the default settings in here to still take effect.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 27, 2019

Member

Should we introduce a -Dmetals.client=coc-metals instead? There might still be coc.nvim users that are not using coc-metals.

This comment has been minimized.

Copy link
@ckipp01

ckipp01 Dec 27, 2019

Author Member

I've thought about that, and I would say that if it moves away any further, that might be a good idea. For now, I think just setting the extra needed values in coc-metals should suffice, and then that way we don't have to add another entry for it. Using coc.nvim without coc-metals should still work fine with what is currently in here.

Maybe once I dig into the tree view stuff, then it'll be a good idea to split it out.

This comment has been minimized.

Copy link
@ckipp01

ckipp01 Dec 27, 2019

Author Member

Thinking about it more, I'll just do it right away. I'll end up doing it anyways, so why not now.

@ckipp01 ckipp01 mentioned this pull request Dec 27, 2019
3 of 3 tasks complete
Copy link
Member

olafurpg left a comment

Looking great! Just a few minor suggestions

docs/editors/vim.md Outdated Show resolved Hide resolved
docs/editors/vim.md Outdated Show resolved Hide resolved

![Run Doctor](https://i.imgur.com/yelm0jd.png)
![Embedded Doctor](https://i.imgur.com/McaAFv5.png)

This comment has been minimized.

Copy link
@olafurpg
docs/editors/vim.md Outdated Show resolved Hide resolved
docs/editors/vim.md Outdated Show resolved Hide resolved
@@ -133,7 +133,6 @@ object MetalsServerConfig {
case "coc.nvim" =>
base.copy(
statusBar = StatusBarConfig.showMessage,
isHttpEnabled = true,

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 27, 2019

Member

Should we introduce a -Dmetals.client=coc-metals instead? There might still be coc.nvim users that are not using coc-metals.

@ckipp01 ckipp01 force-pushed the ckipp01:coc-updates branch from d6f7eaf to 2af2f5f Dec 27, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Dec 27, 2019

Alright, I made the recommended changes to the wording etc, and I also added in a new coc-metals option to the MetalsServerConfig. I wasn't going to do it right away, but this will just continue to change so I might as well do it now.

EDIT (The final time hopefully): After jumping back and forth a bit, I did go with adding in the coc-metals case. Again, I'll need to do it sooner or later, and I've been working on something that will require it, and it will be done sooner than I thought, so I might as well get it in right away.

@ckipp01 ckipp01 requested a review from olafurpg Dec 27, 2019
@ckipp01 ckipp01 force-pushed the ckipp01:coc-updates branch from 2af2f5f to 861ece3 Dec 27, 2019
@ckipp01 ckipp01 force-pushed the ckipp01:coc-updates branch from 861ece3 to bd90b4a Dec 28, 2019
@ckipp01 ckipp01 force-pushed the ckipp01:coc-updates branch from 5732af0 to 0b8d38e Dec 29, 2019
Copy link
Member

olafurpg left a comment

Sounds good. We should have a stable release out soon enough so that most users will be on a version that understands the coc-Metals client

@ckipp01 ckipp01 merged commit 4ab0f2e into scalameta:master Dec 30, 2019
11 of 12 checks passed
11 of 12 checks passed
windows-latest jdk-11 unit tests windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@ckipp01 ckipp01 deleted the ckipp01:coc-updates branch Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.