Skip to content

Conversation

dam9000
Copy link
Contributor

@dam9000 dam9000 commented Mar 11, 2024

This used to be in kickstart, but was removed in the last rewrite, probably to keep the file short.
I would ask you to reconsider and include these keymaps.
I use them regularly and suspect others might find them useful too.

The keymaps are copied from:
https://github.com/lewis6991/gitsigns.nvim?tab=readme-ov-file#keymaps
with added descriptions.

@feoh
Copy link
Collaborator

feoh commented Mar 12, 2024

Hi! I personally have zero problem with this change. These seem like useful features.

However I know @tjdevries explicitly had removing the gitsign stuff in mind with this rewrite, so you may need to take it up directly with him.

One question: Could this be maybe moved into an optional enhancement folks could uncomment to include? I'd do that!

Just trying to figure out if there's middle ground to get everyone what they want.

@dam9000
Copy link
Contributor Author

dam9000 commented Mar 12, 2024

Right, it was removed at this point:
https://www.youtube.com/watch?v=-joJuscbM5w&t=526s
With the explanation of reducing the number of lines of code:

gitsigns is a great plugin, but probably all you need is [the signs character opts]
I don't think we need any of [the keymaps] by default, I don't think anyone is using this...
So if I delete this ... this is 60 lines ...

If we add an "optional" way to enable this it would still increase the amount of code, even more so than if it's not an optional feature. Unless, you're suggesting to comment out the whole block in which case it would count towards the number of comments and not the number of code :)

Anyway it seems we need @tjdevries to make a decision here.

@feoh
Copy link
Collaborator

feoh commented Mar 12, 2024

for what it's worth I spoke to him about this on discord, and he says he'll be looking at PR on this project again in a week or so.

@tjdevries
Copy link
Member

Let me think about it a little bit more, but I'm still leaning towards it adds a lot of noise for newcomers for something that is pretty auxillary. They should probably be focusing on other movements / actions / keymaps /etc instead of thinking about some of these git items.

I wouldn't mind some way to allow them to see these keymaps more easily, or add them but I don't know that it's a good idea to have in the base init.lua

@ro0gr
Copy link

ro0gr commented Mar 15, 2024

I think the "next/prev hunk" keymaps are the only critical ones for the default experience, cause they provide with a way to navigate throgh the signs rendered explicitly by the plugin.

While the rest of features are also cool, I find them being used way less frequently then navigating through the chunks. But maybe it's just me.

@feoh
Copy link
Collaborator

feoh commented Mar 18, 2024

@dam9000 Would you consider making this another optional commented out add in like the linting one we just added?

I can totally see where these features are useful but I also understand and agree with @tjdevries desire to keep the base init.lua lean and mean so it's an easier jumping off point for new folks to actually read and understand.

@dam9000
Copy link
Contributor Author

dam9000 commented Mar 19, 2024

@feoh you mean moving the keymaps to a separate lua/kickstart/plugins/gitsigns.lua which would be commented out? That would kinda work but is not a clean solution because then the lazy spec would be divided into two files (init.lua for the opt.signs and gitsigns.lua for the opt.on_attach). While this works because lazy will merge the tables it will lead to confusion. Also at least the navigation keymaps ([c ]c) should be part of the default config and if that is done then the other keymaps can no longer be split because lazy will not merge the function contents (of course) - only one opt.on_attach hook can be active. Including these keymaps is part of the "good defaults" that attracts people because efficiently navigating git is just as important as navigating the code. (All of the above is of course just my view)

@feoh
Copy link
Collaborator

feoh commented Mar 19, 2024

@dam9000 You're right, that's icky.

If @tjdevries ultimately decides to keep it out, I'll be merging your PR into my fork :) Because if nothing else this functionality would make resolving merge conflicts easier to maintain said fork :)

I'm sorry this has been so choppy and back and forth. I guess this is the way the software donuts get made :)

-- Toggles
map('n', '<leader>tb', gs.toggle_current_line_blame, { desc = '[T]oggle git show [b]lame line' })
map('n', '<leader>td', gs.toggle_deleted, { desc = '[T]oggle git show [d]eleted' })
end,
Copy link

@lewis6991 lewis6991 Mar 20, 2024

Choose a reason for hiding this comment

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

If we want to keep this minimal I would remove:

  • toggle_deleted
  • toggle_current_line_blame
  • <leaderhD> diff against last commit
  • reset_buffer
  • undo_stage_hunk
  • stage_buffer

All these can be done with the :Gitsigns command.

The rest are pretty critical to my workflow: navigation, individual hunk stage/reset, diff, preview, blame.

@dam9000
Copy link
Contributor Author

dam9000 commented Mar 20, 2024

force pushed: rebased to latest master and made a minor simplification of vim.schedule that reduces 4 lines.

-          vim.schedule(function()
-            gs.prev_hunk()
-          end)
+          vim.schedule(gs.prev_hunk)

(same for next_hunk)

Verified that it works correctly.

@dam9000
Copy link
Contributor Author

dam9000 commented Mar 20, 2024

@feoh , @tjdevries here is a PR with reduced list of keymaps and simplified code: #779

@feoh
Copy link
Collaborator

feoh commented Apr 17, 2024

I can appreciate where you're coming from around elegance and cleanliness, but I don't think we're going to see these merged.

I'm running with gitsigns installed as a regular plugin in my fork and it works great.

Would it make sense to not let the perfect be the enemy of the good? I could draft a PR where we make it another optional and by default commented out addition?

@dam9000
Copy link
Contributor Author

dam9000 commented Apr 17, 2024

@feoh yeah I see this is not getting anywhere, if making it an optional plugin would help getting it merged I can prepare that too.

@feoh
Copy link
Collaborator

feoh commented Apr 17, 2024

If you can get to that before I can that would be grand. Thank you for all your efforts - I really appreciate it!

@feoh
Copy link
Collaborator

feoh commented Apr 18, 2024

Closed in favor of #858. Thanks for the hard work on this!

@feoh feoh closed this Apr 18, 2024
@dam9000 dam9000 deleted the pr-gitsigns-keys branch April 18, 2024 04:04
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.

5 participants