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

Add gitsigns essential keymaps #779

Closed
wants to merge 1 commit into from

Conversation

dam9000
Copy link
Contributor

@dam9000 dam9000 commented Mar 20, 2024

An alternative attempt at including only the essential keymaps compared to the full list in: #740
Changes compared to the full list:

  • reduced the list of keymaps as suggested by @lewis6991 (the author of gitsigns):
    (re)Add gitsigns recommended keymaps #740 (review)
  • simplified navigation keymap code - verified that it works also in diff mode, the only difference is a slight change in behaviour as gitsigns navigation does wrap-around while original diff mode does not. Also confirmed with @lewis6991 that it should work.
  • Added a note pointing to gitsigns README for the full list of keymaps

Compared to the original PR this one has about half the lines of code, hopefully this will make it more likely to be included.

@feoh
Copy link
Collaborator

feoh commented Mar 20, 2024

LGTM but I'll defer to @tjdevries as I haven't had a problem with this stuff from jump, but can appreciate the desire to keep things small and understandable.

@feoh
Copy link
Collaborator

feoh commented Mar 21, 2024

Hi!

Just wanted to mention what I did and ask if this might be a good option for you:

feoh@52722b7

If I install gitsigns like a regular old plugin, I get it in all its full fat goodness. Keymaps, configs. The whole enchilada.

Why not just do that? What am I missing? That way @tjdevries can get his super minimal starting point for new folks, and we can enjoy our luxurious git manipulating goodness in our forks? :)

Sorry if I'm missing something obvious, and in NO way do I mean to cut down the value of all your amazing contributions and work on this project.

You really have been an outstanding contributor, and I for one sincerely appreciate it!

Should you ever come to Boston, MA, USA. Please do look me up! I'd love to buy you a beverage or dinner or whatever :)

@dam9000
Copy link
Contributor Author

dam9000 commented Mar 21, 2024

@feoh your suggestion works. But I think it's not clean that a plugin has the spec split into two files. Please open a PR with your solution so there are more options for TJ to pick from. It's not a question what works for me, I have the full enchilada no worries, the question is what is the best for the average or new Neovim user. I think a new Neovim user isn't necessarily a fresh developer, he can be an experienced developer looking for a new editor. A perfect example: DHH moving to Neovim: https://twitter.com/dhh/status/1764340531877105824

Anyway let's wait for @tjdevries feedback and we'll see.

And thanks for the kind words, I have no travel plans to US at the moment, but I'll keep it in mind :)

init.lua Show resolved Hide resolved
@feoh
Copy link
Collaborator

feoh commented Apr 3, 2024

Just coming back to this in my usual PR check:

I don't know that generating a PR of my change is appropriate because my change has the gitsigns plugin installed in lua/custom/plugins which I think we'd prefer to reserve for end users to fill themselves :)

@SebasF1349
Copy link

Gitsigns has just updated and next_hunk() and prev_hunk() has been deprecated. This is the commit with the new suggested way to do keymaps: lewis6991/gitsigns.nvim@59bdc18

Include essential gitsigns keymaps, for more see gitsigns README:
https://github.com/lewis6991/gitsigns.nvim
@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-keys2 branch April 18, 2024 04:02
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.

4 participants