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

fix(pack/rust): Ensure that taplo is only initialized once. #91

Closed
wants to merge 1 commit into from

Conversation

sjcobb2022
Copy link
Contributor

Closes #90

Ensure that taplo is only initialized once.

Just added it to astronvim.lsp.skip_setup.

@sjcobb2022
Copy link
Contributor Author

Sorry for all the commits, i forgot to clean my branch.

@mehalter mehalter force-pushed the main branch 2 times, most recently from 65dfde1 to df1aff6 Compare March 20, 2023 15:18
@mehalter mehalter force-pushed the main branch 2 times, most recently from 4f23ed4 to 3dcc166 Compare March 20, 2023 15:23
Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

@sjcobb2022 I went and fixed the git history on your branch. One thing I should point out is that this is actually not a real "fix". Taplo here is not being initialized twice as a language server. It is having the language server initialize and it is also having the null-ls sources initialize through mason-null-ls. This is disabling the language server and just letting null-ls set it up for linting. This is not the ideal behavior as we probably want the reverse of this. Disabling the null-ls and only using the LSP. I'm going to discuss with the mason-null-ls dev to see the best way to move forward with this.

EDIT: just to clarify, there are no real requests for changes here, just blocking the commit for now until we clear this up

@sjcobb2022
Copy link
Contributor Author

@sjcobb2022 I went and fixed the git history on your branch. One thing I should point out is that this is actually not a real "fix". Taplo here is not being initialized twice as a language server. It is having the language server initialize and it is also having the null-ls sources initialize through mason-null-ls. This is disabling the language server and just letting null-ls set it up for linting. This is not the ideal behavior as we probably want the reverse of this. Disabling the null-ls and only using the LSP. I'm going to discuss with the mason-null-ls dev to see the best way to move forward with this.

EDIT: just to clarify, there are no real requests for changes here, just blocking the commit for now until we clear this up

Alright alright, that sounds good. Feel free to close the pull if you want to move in another direction!

@mehalter
Copy link
Member

Alright alright, that sounds good. Feel free to close the pull if you want to move in another direction!

Ok thanks! I'll keep you updated :D I'm hoping we can get setup_handlers moved to the setup call of mason-null-ls and mason-nvim-dap so that it's easy to override them to not set up different sources easily

@mehalter
Copy link
Member

Good news @sjcobb2022 ! This PR on mason-null-ls will enable us to do what we want ❤️

jay-babu/mason-null-ls.nvim#55

Once this gets merged in I can update this to use it and we can ship this :D

@sjcobb2022
Copy link
Contributor Author

Good news @sjcobb2022 ! This PR on mason-null-ls will enable us to do what we want ❤️

jay-babu/mason-null-ls.nvim#55

Once this gets merged in I can update this to use it and we can ship this :D

Awesome! That sounds great!

Should I close the pull or are we going to just keep this up and edit it.

@mehalter
Copy link
Member

@sjcobb2022 actually, I had a different idea of how we could structure these packs in a more semantically meaningful way, check out #94 . This will make it easier for us to do fixes like this and fix it in all the places where toml language is used like rust + julia. So I am going to go ahead and close this and we can make a new PR that just fixes the toml language pack when that PR comes in. Thanks for catching this, opening an issue, and investigating!!

@mehalter mehalter closed this Mar 22, 2023
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.

fix(pack/rust): Taplo initialized twice in the rust pack
2 participants