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: hack get_keymap too #12

Merged
merged 7 commits into from
May 1, 2023
Merged

fix: hack get_keymap too #12

merged 7 commits into from
May 1, 2023

Conversation

danilshvalov
Copy link
Contributor

@danilshvalov
Copy link
Contributor Author

Since the get_keymap method is now being hacked, the tests do not pass. Here are two options that I see. The first is obviously to remove the falling tests. The second is to come up with something more elegant. But nothing comes to mind so far.

@danilshvalov
Copy link
Contributor Author

Great, I figured out how to fix the tests: use the original functions!

@Wansmer
Copy link
Owner

Wansmer commented May 1, 2023

Hi! Thanks for your contribution!

I tested this PR and it does not seem to work as expected:

  1. The filter_default_keymaps function always returns an empty table instead of filtered. With my config, this behavior breaks neo-tree. Also, I'm installed which-key for testing, and it breaks showing prompt when I press <leader>. I think, the best way to filter original mappings table - it is to see that mapping.desc does not contain LM (prefix of all mappings handled by langmapper).

  2. I think wrappers for get_keymap should not be hacked by default. Better is to just add it to the API and if users need to hack it, they can do it themselves in their config.

@danilshvalov
Copy link
Contributor Author

I checked the method with desc and it doesn't work the way I would like. I will try to fix the filtering function. I've changed something, try again, does the above work for you?

@danilshvalov
Copy link
Contributor Author

I also made them optional. Now you need to activate them as follows:

local langmapper = require("langmapper")
langmapper.setup()
langmapper.hack_get_keymap()

@danilshvalov
Copy link
Contributor Author

With my config, this behavior breaks neo-tree. Also, I'm installed which-key for testing, and it breaks showing prompt when I press <leader>.

It's strange that which-key and neo-tree don't work for you. Everything works as expected for me. I hope the latest patches will fix the problem.

@Wansmer
Copy link
Owner

Wansmer commented May 1, 2023

It's strange that which-key and neo-tree don't work for you. Everything works as expected for me. I hope the latest patches will fix the problem.

I'll test again tonight. Not at the computer anymore.

@Wansmer
Copy link
Owner

Wansmer commented May 1, 2023

Great, now it works! Thanks again!

Fixed #8

@Wansmer Wansmer merged commit 20fc93b into Wansmer:main May 1, 2023
Wansmer added a commit that referenced this pull request May 1, 2023
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.

2 participants