Skip to content

Conversation

kyazdani42
Copy link
Member

NOTE: this should not be merged before we tested it thoroughly.
NOTE2: this should not be merged before the help menu is rewritten
NOTE3: this can be used to print the on_attach migration to the user.

@kyazdani42 kyazdani42 requested a review from alex-courtis July 31, 2022 10:25
@kyazdani42 kyazdani42 force-pushed the chore/migrate-legacy-mappings branch from 64b1387 to 11d97de Compare July 31, 2022 10:26
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

NOTE2: this should not be merged before the help menu is rewritten

We could be lazy: retain the view.mappings.list for help to look up, if we want to merge this before.

NOTE3: this can be used to print the on_attach migration to the user.

Yes. vim.keymap.set is ready to print.


local opts = merge_options(conf)
local netrw_disabled = opts.disable_netrw or opts.hijack_netrw
legacy.move_mappings_to_keymap(opts)
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach: moving the mappings after merge means we only have to do this once.

end
end

local OLD_MAPPING_TABLE = {
Copy link
Member

Choose a reason for hiding this comment

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

We could be lazy and move this to DEFAULT_KEYMAPS with a legacy_action field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, also i've noticed from some PRs on nvim-treesitter that vim.keymap.set takes an optional "desc" argument which we should use so the nvim-tree will work in which-key and allow us to use that in the help menu

@alex-courtis
Copy link
Member

See #1579

@alex-courtis alex-courtis deleted the chore/migrate-legacy-mappings branch December 16, 2022 04:38
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