Closed AndreAugustoDev closed 10 months ago
The upstream branch already reverted #44, so you'll need to rebase first (and please squash the commits).
Please make sure you will not break lsp-status
again as I mentioned in #46
@darkangel-ua
Por favor, certifique-se de que você não vai quebrar
lsp-status
novamente como mencionei em #46
Of course, don't worry about it. We need to test before merging into the main branch Regarding this, I need more information so that I can fix what is causing the error I will endeavor to correct any conflict
Please make sure you will not break
lsp-status
again as I mentioned in #46
@darkangel-ua you can make some tests with latest commit? I really don't know what breaks this
Without going too deeply into the code changes, I would make two recommendations for the structure of this PR:
First, this has a lot of changes, many of them unrelated. I would recommend splitting these changes into ideally separate PRs, but at least separate commits within the PR. Smaller commits are easier to review, and it also makes it easier to iterate on specific areas/features if they need tweaks.
Second, it would help a lot to add more information about the motivation of each of the changes and (if necessary) what the difference is to the user. For example:
Replace setup({ preset }) with setup({ symbols })
Would be great to hear more about why this change is needed and what the setup()
call actually looks like before/after. This is also a good place to explain how you expect to handle backwards-compatibility.
Last note now that I think of it: this is a personal preference and reasonable minds could disagree, but I would avoid forced API name changes just for the sake of cleanup. Every API breakage comes with a cost to users, even if it's a simple fix. Sometimes it's necessary, because you really need to change the underlying structure of the code, or certain config options have been deprecated/replaced/reorganized. If that isn't the case, then I find I prefer to simply do a transparent rename (e.g. M.init = M.setup
), change the documentation, and leave the old version functioning forever.
@stevearc
Without going too deeply into the code changes, I would make two recommendations for the structure of this PR:
First, this has a lot of changes, many of them unrelated. I would recommend splitting these changes into ideally separate PRs, but at least separate commits within the PR. Smaller commits are easier to review, and it also makes it easier to iterate on specific areas/features if they need tweaks.
It's correct, but because the code is small, I didn't see the need to do a PR for each edition Besides, what I did was a refactoring, also because it's small and needs a cleaner code Second, it would help a lot to add more information about the motivation of each of the changes and (if necessary) what the difference is to the user. For example:
Replace setup({ preset }) with setup({ symbols })
I followed the format of the repository commits And in this example, this replacement was made to use a single input for defining symbols, handled by the plugin whether it's a preset or a customization
Would be great to hear more about why this change is needed and what the
setup()
call actually looks like before/after. This is also a good place to explain how you expect to handle backwards-compatibility.The setup() function does the exact same thing as init(), the change made follows what I would say 99% of plugins use the setup() function Last note now that I think of it: this is a personal preference and reasonable minds could disagree, but I would avoid forced API name changes just for the sake of cleanup. Every API breakage comes with a cost to users, even if it's a simple fix. Sometimes it's necessary, because you really need to change the underlying structure of the code, or certain config options have been deprecated/replaced/reorganized. If that isn't the case, then I find I prefer to simply do a transparent rename (e.g.
M.init = M.setup
), change the documentation, and leave the old version functioning forever.
The code already had some deprecated options, I don't think it's useful to keep this code deprecated any longer As for the API change, the preference is for the new revision to be used In this PR there is a deprecated warning when using a deprecated function I believe there is no need to duplicate the function, new users following the README will enjoy the most current one, and old users will see a deprecation notification and need to update the settings These are minimal changes, just a few letters to the end user
Any progress on this? Would like to have missing types such as Namespace
and Package
. Thanks!
Sorry for this confusion, I fixup and doc the breaks made, now its compatible with old commits since settings are updated Add lspkind.symbols in public API Add lspkind.presets in public API
BREAKING CHANGES: Replace lspkind.init() with lspkind.setup() Replace setup({ preset }) with setup({ symbols }) Replace lspkind.symbols_map with lspkind.symbols Use lspkind.init() as deprecation notifier