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

refactor: add warning to deprecated functions #16536

Merged

Conversation

dundargoc
Copy link
Member

@dundargoc dundargoc commented Dec 5, 2021

warnings and functions will be removed for 0.7.0 in a followup PR.

@github-actions github-actions bot added refactor changes that are not features or bugfixes lsp lua stdlib labels Dec 5, 2021
@dundargoc dundargoc marked this pull request as draft December 5, 2021 20:08
@dundargoc dundargoc force-pushed the refactor/remove-deprecated-functions branch from 58f0b73 to 09016dd Compare December 5, 2021 20:14
@dundargoc dundargoc requested review from gpanders and mjlbach December 5, 2021 20:14
@dundargoc dundargoc marked this pull request as ready for review December 5, 2021 20:14
@ii14
Copy link
Member

ii14 commented Dec 5, 2021

Maybe instead of removing vim.lsp.diagnostics, an error could be thrown with some helpful message like Use vim.diagnostic.something().

@gpanders
Copy link
Member

gpanders commented Dec 5, 2021

Maybe instead of removing vim.lsp.diagnostics, an error could be thrown with some helpful message like Use vim.diagnostic.something().

Yes I think this is the better approach (although I would suggest a warning rather than an error).

vim.api.nvim_echo({{'This method is deprecated. Use {replacement} instead', 'WarningMsg'}}, true, {})

And this needs to make it into a full release before we completely remove these. I had wanted to do something like this for the 0.6 release but never got around to it :( So this deprecation warning will be in 0.7 and they can be fully removed in 0.8. That's a long timeline, but that's plenty of runway to ensure we don't pull the rug out from people too quickly.

@clason
Copy link
Member

clason commented Dec 5, 2021

Compromise: add those warnings to the top of each deprecated function in a different PR now and backport to 0.6.1.

After 0.6.1 is released, we can merge this PR and turn the warnings into errors (prettied up via __index metatable or not).

@clason
Copy link
Member

clason commented Dec 5, 2021

Also, tests would need adapting.

@dundargoc dundargoc force-pushed the refactor/remove-deprecated-functions branch from 09016dd to 7157f0c Compare December 5, 2021 21:20
@dundargoc dundargoc changed the title refactor: remove deprecated functions refactor: add warning to deprecated functions Dec 5, 2021
@dundargoc dundargoc force-pushed the refactor/remove-deprecated-functions branch 3 times, most recently from c9416b6 to 71ec6a1 Compare December 6, 2021 10:39
@dundargoc dundargoc requested a review from gpanders December 6, 2021 10:39
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/util.lua Outdated Show resolved Hide resolved
runtime/lua/vim/lsp/util.lua Outdated Show resolved Hide resolved
@zeertzjq

This comment has been minimized.

@clason clason marked this pull request as draft December 6, 2021 12:52
@dundargoc dundargoc force-pushed the refactor/remove-deprecated-functions branch from a0ab607 to e8d2b31 Compare December 6, 2021 12:58
@clason clason marked this pull request as ready for review December 6, 2021 13:02
@dundargoc dundargoc requested a review from zeertzjq December 6, 2021 13:04
Copy link
Member

@zeertzjq zeertzjq left a comment

Choose a reason for hiding this comment

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

There are no tests that depend on deprecated APIs, so I think tests don't need adapting.

@clason
Copy link
Member

clason commented Dec 6, 2021

Yes, with the new approach of adding a warning to instead of completely removing the deprecated functions, the tests should not need any changes.

@clason clason added this to the 0.6.1 milestone Dec 6, 2021
@gpanders
Copy link
Member

gpanders commented Dec 6, 2021

I think save() should be marked deprecated as well (I can't remember why I only marked it as @private).

@dundargoc dundargoc removed the request for review from mjlbach December 8, 2021 13:28
@gpanders gpanders merged commit c4d70da into neovim:master Dec 8, 2021
@gpanders
Copy link
Member

gpanders commented Dec 8, 2021

/backport

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Cannot read property 'match' of null

@clason
Copy link
Member

clason commented Dec 8, 2021

/backport release-0.6

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

4 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

@gpanders
Copy link
Member

gpanders commented Dec 8, 2021

/backport

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Cannot read property 'match' of null

@clason
Copy link
Member

clason commented Dec 8, 2021

/backport

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Successfully created backport PR #16575 for release-0.6.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Git push to origin failed for release-0.6 with exitcode 1

@gpanders
Copy link
Member

gpanders commented Dec 8, 2021

For posterity (and any curious passers by wondering at the carnage that occurred here): empty PR descriptions will cause the backport workflow to fail with Cannot read property 'match' of null (korthout/backport-action#175). Always describe your PRs kids!

@dundargoc dundargoc deleted the refactor/remove-deprecated-functions branch December 8, 2021 18:40
dmitmel pushed a commit to dmitmel/neovim that referenced this pull request Dec 26, 2021
kiprasmel added a commit to kiprasmel/neovim that referenced this pull request Jan 5, 2022
@zeertzjq zeertzjq modified the milestones: 0.6.2, 0.6.1 Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp lua stdlib refactor changes that are not features or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants