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

fix(keymap.helpers): improve _toggle* helper functions #1397

Merged
merged 3 commits into from
Jan 30, 2025
Merged

Conversation

Jint-lzxy
Copy link
Collaborator

This commit refined the two _toggle* functions to cut down redundancy, reworded msgs, and made sure 0/1 are not used in _toggle_diagnostic bc it's bug-prone.

Also, what yall think about using <leader>t for these? Cuz this seems quite unique compared to other LSP keymaps...? Also the term "toggle in buffer" feels off and may be better as "(global) toggle" IMO

This commit refined the two `_toggle*` functions to cut down redundancy,
reworded msgs, and made sure 0/1 are not used in `_toggle_diagnostic` bc it's
bug-prone.

Signed-off-by: Jint-lzxy <[email protected]>
@ayamir
Copy link
Owner

ayamir commented Jan 19, 2025

I rarely use this keymap, use <leader>t for test in my own config.

@AngelontheRoad
Copy link
Contributor

AngelontheRoad commented Jan 21, 2025

Yeah they are global toggled keymaps not buffer keymaps . I'm sorry for my fault...

The keymaps are map to <leader>t for ease of memory. I think it's better to move them to editor.lua bc it's designed to clean off the editor if there're too many warnings and long inlay hints.

@Jint-lzxy
Copy link
Collaborator Author

The keymaps are map to <leader>t for ease of memory.

My two cents here is I don't think this rlly fits with other keymaps bc these two are specific to LSP and should only be active when a language server is actually attached to the buffer. That's why I'm thinking it might make more sense to use something like <leader>lv for virtual text and <leader>lh for inlay hints, keeping them within the LSP category (<leader>l*) and still making them easy to remember.

I think it's better to move them to editor.lua bc it's designed to clean off the editor if there're too many warnings and long inlay hints.

Given the above I think it'd make more sense to move them to lsp.lua...? Otherwise what would these keymaps do if, for example, we're editing a text file and want to disable inlay hints? It just doesn't quite add up to me.

@AngelontheRoad
Copy link
Contributor

Given the above I think it'd make more sense to move them to lsp.lua...? Otherwise what would these keymaps do if, for example, we're editing a text file and want to disable inlay hints? It just doesn't quite add up to me.

My point is that you won't use them editing a text file bc there's no lsp information at all, and most importantly, there's no error while using these keymaps on editing a non-lsp-attached file.

Moving these keymaps to lsp.lua complys with the keymaps' source of LSP, but may cause confusion with the LSP keymaps in completion.lua. To be serious, It's more appropriate to move them in a subfolder of editor , like keymap/editor/lsp.lua, which is much more complicated, obviously. Furthermore, what if we add one or more keymaps about treesitter or other editor plugins which not work on some text file too, should we also create a single file for them? Due to their simple functionality, I think it's OK to stay them in editor.lua and just add a comment above.

My two cents here is I don't think this rlly fits with other keymaps bc these two are specific to LSP and should only be active when a language server is actually attached to the buffer. That's why I'm thinking it might make more sense to use something like lv for virtual text and lh for inlay hints, keeping them within the LSP category (l*) and still making them easy to remember.

<leader>l* is good for me, too. Your point is on LSP and mine is on toggle. It's both OK for me.

@ayamir
Copy link
Owner

ayamir commented Jan 26, 2025

Agree to modify these keymaps to <leader>lv and <leader>lh b/c it's lsp-related. t can have more wide range of uses like test.

@ayamir ayamir merged commit 8ea96c1 into main Jan 30, 2025
4 checks passed
@ayamir ayamir deleted the fix/vt-diag-func branch January 30, 2025 02:06
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.

3 participants