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

chore(notice): Breaking change notice #2078

Closed
wants to merge 1 commit into from

Conversation

ranjithshegde
Copy link
Contributor

Commit #1984 included a breaking change.
Function require("lspconfig").available_servers has been moved to the
utils module. The new call is
require("lspconfig.util").available_servers

@ranjithshegde
Copy link
Contributor Author

ranjithshegde commented Aug 23, 2022

The PR #1984 was commit before I could push the breaking change notice from my local.
Sorry about the mess. With more than 80 messages, I was too slow to notice this omission.

Is this the best strategy of notifying users? Or is there a different preferred method?

@glepnir
Copy link
Member

glepnir commented Aug 23, 2022

like i said keep it in lspconfig will be better. and your commit message is wrong check the Contribut.md

@ranjithshegde
Copy link
Contributor Author

like i said keep it in lspconfig will be better. and your commit message is wrong check the Contribut.md

That part was not my decision. It was the original maintainer mjlbach. I simply continued the work.
Besides, as discussed in the PR messages, this massively simplifies the logic. lspconfig configures the servers, plugin adds some commands and utils has some useful functions

BREAKING CHANGE: Functions from `lspconfig.lua`

Commit neovim#1984 included a breaking change.
Function `require("lspconfig").available_servers` has been moved to the
utils module. The new call is
`require("lspconfig.util").available_servers`
@glepnir
Copy link
Member

glepnir commented Aug 23, 2022

the plugin/lspconfig should be simple .it shouldn't have much functions .

and part for what? don't think move to utils.lua is correct.

@ranjithshegde ranjithshegde changed the title BREAKING CHANGE: Functions from lspconfig.lua chore(notice) Aug 23, 2022
@ranjithshegde ranjithshegde changed the title chore(notice) chore(notice): Breaking change notice Aug 23, 2022
@justinmk
Copy link
Member

Yes, I noticed that #1984 was possibly exposing some functions as "public". I merged it to get a baseline. In general, as @glepnir said we want to remove things from lspconfig, not add.

@ranjithshegde
Copy link
Contributor Author

No new functions were made public that weren't already. They were simply inside lspconfig.lua. Also nothing new was added. Just reworked the logic.
What do you advice as per changes?

@justinmk
Copy link
Member

justinmk commented Aug 23, 2022

No new functions were made public that weren't already

No problem then :) Need others who are looking closely at the code to suggest what can be removed for 0.8.

@glepnir
Copy link
Member

glepnir commented Aug 23, 2022

we usually develop a plugin the structure will be like

├── lua
│   └── plugin_name
│   │   ├── utils.lua  or other name.lua        -- some common utility functions
│   └── plugin_name.lua.       -- main 
├── plugin
│   └── plugin_name.lua   -- highlight / commands / some necessary autocmd events

the plugin/plugin_name.lua should be keep simple not much functions define in this file unless it's necessary. look at current plugin/lspconfig.lua It should be more clear and clean . like #2051 plugin/lspconfig.lua it only have one nessary function .

about utils.lua . Obviously it's not necessary. Also the name is not fixed. Everyone has their own name ideas such as libs and so on. But usually this file only have common utility functions . what is common utility functions ? In my opinion it's a wrapper around some generic function. It does not have any connection to the main variables of the program. check the before utils.lua it work like i said. Any function does not have any connection with server-related variables . look at current the utils.lua some functions move from lua/lspconfig.lua to utils. I don't know this logic. and it may broken some configs or plugins. just search on github . I don't think this break change is improve . use the new api to replace old code is necessary . but any improve should consider compatibility and whether it is the correct logic. all above just my person opinion , not relate the team . (btw all about this I had talk about on your pr when I review your change as maintainer but you don't reply me many times)

image

@ranjithshegde
Copy link
Contributor Author

ranjithshegde commented Aug 23, 2022

(btw all about this I had talk about on your pr when I review your change as maintainer but you don't reply me many times)

I re-read the reviews and messages, I still could not find your review, except the one on codespell. Could you please point them for me?

As to the breaking change. That is the purpose of this PR. To notify users. The other PR was in wait for a long time as a trial period, first by mjlbach, and a couple of months ago by me. As you can see it received numerous reviews from many maintainers.

I don't think this break change is improve . use the new api to replace old code is necessary . but any improve should consider compatibility and whether it is the correct logic.

I stand by its logic. It does simplify the core functionality of the project. Also, in future commits, most of the utility functions will be deprecated because many of those functions are already in neovim core. Check vim.fs module.

I will create a tracking issue soon with a list of proposed deprecation or removal for discussion with you and the rest of the community.

Here is a draft issue #2079

@ranjithshegde
Copy link
Contributor Author

With #2080, this is no longer necessary. Thanks everyone.

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.

3 participants