Skip to content

Commit

Permalink
fix: staged_signs with legacy signs
Browse files Browse the repository at this point in the history 
Fixes #798
  • Loading branch information
@lewis6991
lewis6991 committed May 25, 2023
1 parent c18b7ca commit f868d82
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 14 deletions.
15 changes: 8 additions & 7 deletions lua/gitsigns/signs/vimfn.lua
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 8 additions & 7 deletions teal/gitsigns/signs/vimfn.tl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ end
local sign_define_cache: {string:table} = {}
local sign_name_cache: {string:string} = {}

local function get_sign_name(stype: string): string
if not sign_name_cache[stype] then
sign_name_cache[stype] = string.format(
'%s%s', 'GitSigns', capitalise_word(stype))
local function get_sign_name(name: string, stype: string): string
local key = name..stype
if not sign_name_cache[key] then
sign_name_cache[key] = string.format(
'%s%s%s', 'GitSigns', capitalise_word(key), capitalise_word(stype))

This comment has been minimized.

Sign in to view

Copy link
@calops

calops May 26, 2023

That's a breaking change 😬
Should be marked as such in the commit message maybe?

This comment has been minimized.

Sign in to view

Copy link
@lewis6991

lewis6991 May 26, 2023

Author Owner

This is not breaking. The sign names are not a part of the public API. You must use setup() to configure signs.

This comment has been minimized.

Sign in to view

Copy link
@calops

calops May 26, 2023

I understand that this might not be the intention, but the sign names are public entities freely accessible from within neovim. They're designed to be part of the general signs API, and the whole point of this is to enable seamless interoperability between plugins.

It seems reasonable to me that somebody would want to integrate with git-signs in such a way, for example when building a custom statuscolumn (which is my use-case). I see no way of doing this without using the sign names.

I completely understand why the change here is wanted, but I definitely think it's a breaking one.

This comment has been minimized.

Sign in to view

Copy link
@lewis6991

lewis6991 May 26, 2023

Author Owner

I understand that this might not be the intention, but the sign names are public entities freely accessible from within neovim.

By that logic, then almost every single commit can be classed as a breaking change since all the Gitsigns Lua modules are exposed (because it's Lua). Whenever any of the exported functions from any of these modules change, then something freely accessible has changed and thus must be breaking.

Thankfully, this is not what defines a breaking change. Only changes to the documented API are classed as breaking.

They're designed to be part of the general signs API, and the whole point of this is to enable seamless interoperability between plugins.

This is not true. The signs API provided by Neovim is designed to provide UI. It has never been intended for interoperability between plugins.

To add, Gitsigns supports using extmarks as its signs backend which will be enabled by default in the future. This is exactly why signs have to be configured in setup(). No API has been provided for inspecting Gitsigns signs other than get_hunks().

It seems reasonable to me that somebody would want to integrate with git-signs in such a way, for example when building a custom statuscolumn (which is my use-case). I see no way of doing this without using the sign names.

Nope. It is totally unreasonable because you are depending on something that has no guarantees attached to it.

I completely understand why the change here is wanted, but I definitely think it's a breaking one.

As the plugin author, I get to decide what is breaking and what isn't since I decide what the public API is. Just because something is exposed, does not make it API.

This comment has been minimized.

Sign in to view

Copy link
@calops

calops May 26, 2023

Fair enough, my apologies if I sounded presumptuous.

Do you have any suggestion for a stable way to achieve what I'm trying to do though? I want to implement custom behaviors for signs coming from this plugin specifically, but the only surface area I can find is the name of the signs. Perhaps some functions to get all the chunks identified by gitsigns in a buffer?

This comment has been minimized.

Sign in to view

Copy link
@lewis6991

lewis6991 May 26, 2023

Author Owner

Perhaps some functions to get all the chunks identified by gitsigns in a buffer?

You can use get_hunks() for that.

Other than that, we would need to add something.

Internally Gitsigns manages signs with a Sign type which contains the fields: type, count, and lnum. We could potentially expose that.
end

return sign_name_cache[stype]
return sign_name_cache[key]
end

local function sign_get(name: string): table
Expand All @@ -57,7 +58,7 @@ local function define_signs(obj: B, redefine: boolean)
-- Define signs
for stype, cs in pairs(obj.config) do
local hls = obj.hls[stype]
define_sign(get_sign_name(stype), {
define_sign(get_sign_name(obj.name, stype), {
texthl = hls.hl,
text = config.signcolumn and cs.text or nil,
numhl = config.numhl and hls.numhl or nil,
Expand Down Expand Up @@ -107,7 +108,7 @@ function M:add(bufnr: integer, signs: {M.Sign})
local to_place = {}

for _, s in ipairs(signs) do
local sign_name = get_sign_name(s.type)
local sign_name = get_sign_name(self.name, s.type)

local cs = self.config[s.type]
if config.signcolumn and cs.show_count and s.count then
Expand Down

0 comments on commit f868d82

Please sign in to comment.