sindrets / diffview.nvim

Single tabpage interface for easily cycling through diffs for all modified files for any git rev.
Other
3.74k stars 105 forks source link

use `nvim_get_hl` if available #326

Closed 3699394 closed 1 year ago

3699394 commented 1 year ago

use latest api if available https://github.com/neovim/neovim/pull/22693

mikesmithgh commented 1 year ago

Tested these changes locally and can confirm they fixed #327 for me.

Awesome, thanks!

sindrets commented 1 year ago

Thanks for the PR! I don't have time to properly review this for about a week. But after a quick look it seems like the new API returns slightly different data than the old nvim__get_hl_defs() (some of the keys have been renamed). So although your suggested change might avoid the error, it might not be the correct way to accommodate this new API. I will have to look closer into the matter when I have time.

mehalter commented 1 year ago

@sindrets this is correct, to help with some insight. The old approach returns properties foreground and background, the new API returns these as fg and bg

TisnKu commented 1 year ago

Thanks for the PR.

mosheavni commented 1 year ago

Guys, what's the hold up? the plugin is completely broken now. Can this be merged already? Thanks.

gegoune commented 1 year ago

@mosheavni Be patient. https://github.com/sindrets/diffview.nvim/pull/326#issuecomment-1484093994 Use your package manager to pin to latest working version in meantime.

jamestrew commented 1 year ago

You can subscribe to this PR to get notified of when it gets merged as well.

Breaking bugs like this is par for the course when riding neovim nightly so let's exercise some patience. You can always switch back to stable if you want to.

mikesmithgh commented 1 year ago

You can subscribe to this PR to get notified of when it gets merged as well.

Breaking bugs like this is par for the course when riding neovim nightly so let's exercise some patience. You can always switch back to stable if you want to.

100% agree. If you are on nightly, you should expect occasional breaking changes.

c3n21 commented 1 year ago

Guys, what's the hold up? the plugin is completely broken now. Can this be merged already? Thanks.

While the maintainer is merging this PR if you are using bob you can pin this version ea0b66d which is the commit before the one that introduces this change

3699394 commented 1 year ago

Guys, what's the hold up? the plugin is completely broken now. Can this be merged already? Thanks.

you can patch hl.lua with my pr, that's what I have been doing

akinsho commented 1 year ago

@3699394 I think you need to update (EDIT: add new clauses I meant) these as well, since hl now returns {bg = value, fg = value, sp = value }

https://github.com/sindrets/diffview.nvim/blob/58035354fc79c6ec42fa7b218dab90bd3968615f/lua/diffview/hl.lua#L134-L136

as per

Thanks for the PR! I don't have time to properly review this for about a week. But after a quick look it seems like the new API returns slightly different data than the old nvim__get_hl_defs() (some of the keys have been renamed). So although your suggested change might avoid the error, it might not be the correct way to accommodate this new API. I will have to look closer into the matter when I have time.

sindrets commented 1 year ago

Superseded by #335.

Like I suggested, the fix required here was more involved than simply replacing what API function were called. Some of the attribute keys returned from the new API have been renamed. I also worked on a fix upstream related to retrieving and setting non-translated highlight attributes for linked groups with the new API functions.

Regardless, @3699394 thanks for your work!

mikesmithgh commented 1 year ago

Superseded by #335.

Like I suggested, the fix required here was more involved than simply replacing what API function were called. Some of the attribute keys returned from the new API have been renamed. I also worked on a fix upstream related to retrieving and setting non-translated highlight attributes for linked groups with the new API functions.

Regardless, @3699394 thanks for your work!

Thanks! I just tested it out successfully 👍