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

Avoid using deprecated nerdfont characters #64

Merged
merged 3 commits into from
May 4, 2023

Conversation

leiserfg
Copy link
Contributor

No description provided.

@carlosala
Copy link
Contributor

@onsails could we merge here? It's a really great improvement for nerd fonts version >3.

@Kedap
Copy link

Kedap commented May 1, 2023

There are still some deprecated icons in your array
Captura de pantalla_2023-05-01_15-20-24

Nerdfix may help

@leiserfg
Copy link
Contributor Author

leiserfg commented May 1, 2023

There are still some deprecated icons in your array

I just fixed the ones I noticed (because they clashed with Noto).

Copy link

@rj1 rj1 left a comment

Choose a reason for hiding this comment

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

thanks!

@anthony-S93
Copy link

anthony-S93 commented May 3, 2023

Hello there, @onsails. Just want to ask if this plugin is still being maintained. So far, there are 3 un-merged pull requests, the oldest of which dates as far back as last year.

Thanks for the plugin and cheers.

@leiserfg
Copy link
Contributor Author

leiserfg commented May 3, 2023

In the meantime, feel free to use https://github.com/leiserfg/lspkind.nvim/ I won't be adding more changes (as lspkind is "done") but it has the new icons.

@carlosala
Copy link
Contributor

some icons are not right in this PR though. I do recommend using nerdfix in both readme and init.lua and accepting first suggestion (which turns out to be the real replacement)

@leiserfg
Copy link
Contributor Author

leiserfg commented May 3, 2023

some icons are not right in this PR though. I do recommend using nerdfix in both readme and init.lua and accepting first suggestion (which turns out to be the real replacement)

I used nerdfix to get the replacements.

@cebor
Copy link

cebor commented May 4, 2023

some icons are not right in this PR though. I do recommend using nerdfix in both readme and init.lua and accepting first suggestion (which turns out to be the real replacement)

Text, Field and Class look a bit different now, but i think that's intended.

Not sure about Unit.

@anthony-S93
Copy link

Text, Field and Class look a bit different now, but i think that's intended.

Class looks a bit different this time because @leiserfg used the codicon class symbol (codepoint eb5b) instead of the material design "shape" icon (codepoint f0831), which was the original.

@leiserfg
Copy link
Contributor Author

leiserfg commented May 4, 2023

Text, Field and Class look a bit different now, but i think that's intended.

Class looks a bit different this time because @leiserfg used the codicon class symbol (codepoint eb5b) instead of the material design "shape" icon (codepoint f0831), which was the original.

I'll change it to the material one if that is a big deal.

@anthony-S93
Copy link

I'll change it to the material one if that is a big deal.

It's not that big of a deal at all.

@cebor
Copy link

cebor commented May 4, 2023

Which would be a big deal if the author @onsails reports back. =)

@onsails
Copy link
Owner

onsails commented May 4, 2023

Sorry for the delay guys, have been traveling a lot. Merged!

@onsails onsails merged commit a6b84cb into onsails:master May 4, 2023
@hjdarnel
Copy link

I think it's worth calling out that this is a breaking change unless you have installed a nerdfont with v3 glpyhs. Dunno how one would've prevented that, but I stumbled across this as I synced my packages but didn't update any fonts, which broke it.

@ElPiloto
Copy link

Came here cause I had to do sleuthing to realize that this was the cause of my config breaking. Many things could have been done to mitigate this breaking change. Here are some off the top of my head:

  1. Keep both the v2 and v3 configuration and have users opt into v3 via a config setting instead of breaking existing users on v2.
  2. Open a separate branch with v3 and point users to v3 that branch in the README instead of breaking existing users on v2.
  3. Have a period with a deprecation message.
  4. Have a breaking changes section on the README or breaking changes issue.

In any case, thanks for the contributions. Next time try to do it in a more thoughtful way.

@leiserfg
Copy link
Contributor Author

In any case, thanks for the contributions. Next time try to do it in a more thoughtful way.

IMHO this is not an issue, you can:

  • update the font and lspkind
  • not update the font and lock the version of lspkind before the font update.

@ElPiloto
Copy link

Good for you that you don't think breaking changes are a problem! Some of us do.

@carlosala
Copy link
Contributor

@ElPiloto what is breaking is that it stopped working for most users that had packages updated, it was important to change it :(

@ElPiloto
Copy link

@carlosala The claim here is that "it stopped working for most users that had packages updated." The breaking changes from nerdfont v3 happened 1 month ago so your claim amounts to saying "most lspkind users have updated their nerdfont in the last month." At the very least that claim needs to be supported. But my intuition says most users aren't updating their fonts on a weekly or monthly basis.

But that's besides the point really: the breaking change in nerdfonts v3 was propagated as another breaking change here. There are graceful ways to deal with breaking changes, none of which were deployed here (it wasn't even flagged as a breaking change anywhere by the commit author). And when I did say suggest dealing with it gracefully in the future, the response was basically "breaking changes don't matter: either pin your version or update your fonts." But again, some people do think breaking changes are a problem. Here is an example of someone else's plugin where they do care about introducing breaking changes gracefully. There are plenty more examples on the internet.

@carlosala
Copy link
Contributor

carlosala commented May 27, 2023

@ElPiloto from v2.3.0 it's not breaking actually, January 18th was the release.
But yeah, you are right in terms of breaking changes

@leiserfg
Copy link
Contributor Author

leiserfg commented May 27, 2023

I get the point of avoiding breaking changes as much as possible, if you check LuaSnip you will see all the internal workarounds we have done there to make it as backward-compatible as we could. But in this case, you spent more energy discussing than the one required for updating the font, and/or creating an issue that could be then pinned for reference for any other user with the same problem.
In any case sorry for the annoyance, but please, chill out.

@sharpicx
Copy link

some icons are not right in this PR though. I do recommend using nerdfix in both readme and init.lua and accepting first suggestion (which turns out to be the real replacement)

I used nerdfix to get the replacements.

how did u use nerdfix to solve those problems? im still confused

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.

10 participants