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

plugins/cmp: remove nvim-cmp deprecations warnings #2642

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HeitorAugustoLN
Copy link
Member

These deprecations were introduced in February 2021 with a planned removal in June 2024. These deprecated options have been in place long enough for users to migrate to the new configuration format.

These deprecations were introduced in February 2021 with a planned
removal in June 2024. These deprecated options have been in place
long enough for users to migrate to the new configuration format.
@HeitorAugustoLN HeitorAugustoLN marked this pull request as ready for review December 11, 2024 22:17
@MattSturgeon
Copy link
Member

MattSturgeon commented Dec 11, 2024

I know the "TODO" comments kinda imply otherwise, but I'm actually in favour of keeping deprecation warnings indefinitely. Especially rename aliases.

I see the "remove after" as more of a "do not remove until at least" rather than a "don't forget to clean this up". I.e. the earliest date the can be removed, not the time they should be removed.

I'm fine with removing deprecations that both have served their time and are actively causing some problem. But most deprecations do no harm,1 and therefore have no reason to be removed.

These particular deprecations are a good example of how they can be kept with minimal clutter; they are in a dedicated file that can usually be ignored while browsing the codebase.

I raised a discussion thread on this a while back #2183, although it never got any attention 😅

Footnotes

  1. Maybe there's some performance impact, but I haven't measured this. My assumption is that lazy-eval means they have very little impact on eval time.

@khaneliman
Copy link
Contributor

I am actually curious about evaluation time. I was going to clean up some deprecations but I agree that some of them we can just shove off to separate deprecation files and hide for a bit if there isn't a negative effect in their existence. I think a good first step would be moving more deprecations to these separate files that get included and then evaluate which ones should actually be removed

@HeitorAugustoLN
Copy link
Member Author

I hadn't thought about keeping deprecations indefinitely, but your points make sense, especially if they're isolated and not causing issues. I stumbled upon these deprecations while working on my other PR, and removing them felt like a straightforward cleanup task given their long grace period.

@MattSturgeon
Copy link
Member

I am actually curious about evaluation time.

Me too, but I don't know how to effectively measure this.

As an anecdote to highlight how amazing lazy-eval really is, notice how when you eval & build your nixos config you only have to eval the parts of nixos you are actually using in your config. If you had to eval all of nixos, your config would take much longer to eval before it could be built!

To take this example furture, notice how you can evaluate individual parts of your config much faster than you can eval config.system.build.toplevel.

E.g. this is pretty snappy

$ nix eval .#nixosConfigurations.desktop.config.users.users --apply 'builtins.attrNames'

And this is even faster

$ nix eval .#nixosConfigurations.desktop.config.system.name

But this is a bit slower

$ nix eval .#nixosConfigurations.desktop.config.system.build.toplevel

(note, doing this with the oldschool nix-instantiate --eval -E instead of nix eval would be even faster, because flakes have to copy everything to the nix store before they even begin evaluating)

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