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

feat: add support for sourcery #1495

Merged
merged 3 commits into from
Mar 8, 2022
Merged

feat: add support for sourcery #1495

merged 3 commits into from
Mar 8, 2022

Conversation

kylo252
Copy link
Contributor

@kylo252 kylo252 commented Nov 27, 2021

Description

Add support for https://github.com/sourcery-ai/sourcery

Related discussions: LunarVim/LunarVim#850 and sourcery-ai/sourcery#43


TODO

  • make sure it's clear that the token is required
  • find some testers who already use the service

@Uzaaft
Copy link
Contributor

Uzaaft commented Nov 29, 2021

I am already using the service if you`d like a tester.

@Uzaaft
Copy link
Contributor

Uzaaft commented Dec 21, 2021

Any updates regarding this PR?

@kylo252
Copy link
Contributor Author

kylo252 commented Dec 22, 2021

@Uzaaft, I was waiting for your tests results 😅

@Uzaaft
Copy link
Contributor

Uzaaft commented Dec 22, 2021

Huston, we had a little miscommunication here. xD I'll test it ASAP. Hopefully within 12h.

@Uzaaft
Copy link
Contributor

Uzaaft commented Dec 22, 2021

@kylo252 The sourcery token is often located in the path: ~/.config/sourcery/auth.yaml file.
I propose that we remove the token part from the config(I did it locally). Makes it more secure as well regarding pushing your config to github.
Edit: Regarding user experience if we were to the token, I can create an markdown file for the config with instructions and open a PR to your repository if you`d like.

@Uzaaft
Copy link
Contributor

Uzaaft commented Dec 22, 2021

Other than that, it works beautifully <3

init_options = {
editor_version = 'vim',
extension_version = 'vim.lsp',
token = '<YOUR_TOKEN>',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should a remove the token section, and instead require that people use the auth.yaml file in the ~/.config/sourcery/auth.yaml file. Sourcery even gives the user the command: sourcery login to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can it not work with a local auth.yaml? otherwise, where's the path for Windows? I can't find that info on their site, could you please share the link?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I guess it can. But from what I see, I can't understand where the token is used, besides the variable assignment

token = "<YOUR_TOKEN>"

Unless I am wrong, and it is used inside the mechanics of lspconfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can it not work with a local auth.yaml? otherwise, where's the path for Windows? I can't find that info on their site, could you please share the link?

What is considered a "local" auth.yaml file?
Also, output from sourcery login --help:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The best docs I found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was specifically asking about the docs where they mention ~/.config/sourcery/auth.yaml since I couldn't find it anywhere, because for Windows it would have to be something like ~/AppData/Local/sourcery/..

I guess the local file would be .sourcery.yaml? but I can't find if it has anything to do with storing the auth token.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. I'll have to look around. But why use the path? Once it is setup(i.e i am logged in), I dont have to use the Path again. Sourcery fetches the token and saves it for future usage. I.e the LSP works everything after the initial setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. Turns out there are no docs on this...Which is a bad thing. Opened an issue regarding this.

@tcp
Copy link

tcp commented Dec 30, 2021

If you need another tester, I can help :)

@Uzaaft
Copy link
Contributor

Uzaaft commented Dec 30, 2021

It is easy to test. If you are using packer, just change the repository from which you pull the source code of nvim-lspconfig to the one in the PR, and setup the lspconfig with the right token.

@cjber
Copy link
Contributor

cjber commented Feb 11, 2022

There is a typo sourcey -> sourcery which means this branch does not work correctly: https://github.com/kylo252/nvim-lspconfig/blob/c45b65667b7603e246f8a53da5e0950ee1397a3e/lua/lspconfig/server_configurations/sourcery.lua#L14

Is there any progress being made on this PR? I agree the auth.yaml should be used for the token, but unsure how to implement myself.

@kylo252
Copy link
Contributor Author

kylo252 commented Feb 11, 2022

There is a typo sourcey -> sourcery which means this branch does not work correctly

Thanks for catching that, fixed!

Is there any progress being made on this PR? I agree the auth.yaml should be used for the token, but unsure how to implement myself.

Basically waiting on #1495 (comment), unless you guys have some suggestions for auth handling.

@Uzaaft
Copy link
Contributor

Uzaaft commented Feb 11, 2022

Auth.yaml hasn't been added to the docs. I've been stalking the docs for a few weeks now.

@Uzaaft
Copy link
Contributor

Uzaaft commented Feb 12, 2022

Sent sourcery.ai an email since their maintainer on GitHub didn't answer.

@Uzaaft
Copy link
Contributor

Uzaaft commented Feb 12, 2022

@kylo252 I think what you have currently is good enough for usage, despite auth.yaml perhaps being a good way to avoid pushing tokens to GitHub. We can't be stuck just because the a maintainer of an external repo does not answer.

@kylo252 kylo252 marked this pull request as ready for review February 12, 2022 11:02
},
on_new_config = function(new_config, _)
if not new_config.init_options.token then
vim.notify_once('The authentication token must be provided in config.init_options', vim.log.levels.ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefix all lspconfig notifications with [lspconfig], and you should mention what the notification is for, whatever the server name is. Check the capitalization on lspconfig in the other notification (on mobile). Also I think we added notify once after 0.6.1 was backported so fallback to notify if not available

Copy link
Contributor Author

@kylo252 kylo252 Feb 21, 2022

Choose a reason for hiding this comment

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

BTW, the server would throw an error message. That's why I hadn't originally added the handling for it.

@mjlbach mjlbach merged commit 837e273 into neovim:master Mar 8, 2022
@kylo252 kylo252 deleted the add-sourcery branch May 13, 2022 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants