Skip to content

Add getmaps() builtin#10273

Closed
errael wants to merge 1 commit intovim:masterfrom
errael:AddGetmapsBuiltin
Closed

Add getmaps() builtin#10273
errael wants to merge 1 commit intovim:masterfrom
errael:AddGetmapsBuiltin

Conversation

@errael
Copy link
Contributor

@errael errael commented Apr 24, 2022

No description provided.

@codecov
Copy link

codecov bot commented Apr 24, 2022

Codecov Report

Merging #10273 (e1918ae) into master (ac92ab7) will decrease coverage by 0.02%.
The diff coverage is 88.00%.

@@            Coverage Diff             @@
##           master   #10273      +/-   ##
==========================================
- Coverage   81.00%   80.97%   -0.03%     
==========================================
  Files         161      161              
  Lines      185553   185590      +37     
  Branches    41946    41956      +10     
==========================================
- Hits       150301   150289      -12     
- Misses      22742    22786      +44     
- Partials    12510    12515       +5     
Flag Coverage Δ
huge-clang-none 82.44% <88.00%> (+<0.01%) ⬆️
linux 82.44% <88.00%> (+<0.01%) ⬆️
mingw-x64-HUGE 0.00% <0.00%> (ø)
mingw-x64-HUGE-gui 77.23% <90.24%> (-0.05%) ⬇️
windows 76.02% <90.24%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/evalfunc.c 90.62% <ø> (ø)
src/map.c 88.29% <88.00%> (-0.13%) ⬇️
src/os_mswin.c 50.97% <0.00%> (-1.53%) ⬇️
src/gui_w32.c 34.80% <0.00%> (-0.59%) ⬇️
src/terminal.c 77.39% <0.00%> (-0.30%) ⬇️
src/clientserver.c 77.24% <0.00%> (-0.23%) ⬇️
src/os_win32.c 57.11% <0.00%> (-0.11%) ⬇️
src/gui.c 72.92% <0.00%> (-0.10%) ⬇️
src/channel.c 83.82% <0.00%> (-0.09%) ⬇️
src/version.c 84.16% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac92ab7...e1918ae. Read the comment docs.

:echo getloclist(5, {'filewinid': 0})


getmaps() *getmaps()*
Copy link
Member

Choose a reason for hiding this comment

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

All the map related functions start with the map prefix. To be consistent, this function should
be called mapget().

Copy link
Contributor

Choose a reason for hiding this comment

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

Except hasmapto(). It would be nice to start with "map", but what to call it then? mapgetall() ? maplist()?
mapget() suggests getting one mapping.
I used getmappings() for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the presence of getloclist I'd say getmaplist is the name that fits best the existing functions.

getmaps() *getmaps()*
Returns a |List| of all mappings. Each List item is a |Dict|
as defined by |maparg()|. For a given mapping, the Dict from
getmaps() is identical to the dict from |maparg()|.
Copy link
Member

Choose a reason for hiding this comment

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

A user can use the Dict returned by getmaps() and call mapset() to recreate the mapping?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think the {mode} argument to mapset() could be a special value and then the "mode" from the dict is uesd.
The new function should also have an "abbr" optional argument to get a list of abbreviations instead of mappings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test does assert_equal(getmaps_entry, maparg_return) for all mappings; how could it not work? (Famous last words)

have an "abbr" optional argument

And here we go... :-)

So getmappings([{abbr}]), if abbr true, then return the abbreviations.

It could be getmappings({dict}), where the key abbr can be true/false. Just in case there's more options on the way. With that in mind, I'm in favor of just an optional bool for abbreviations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would think the {mode} argument to mapset() could be a special value and then the "mode" from the dict is uesd.

I would suggest mapset({dict}) means take abbr and mode from dict, that's easy for a user to understand.

It's unclear to me, whether or not mapcheck() would want a similar change.

Either way, is there a reason not to include "abbr: true/false" in the mapping-dict? I think it could be done right now, without any negative consequences. I'm guessing mapset() wouldn't care today.

I'm a casual vim user, so I'm mostly clueless about a lot of vim stuff. Considering the above, and playing with the new maplist() function, I did the following experiment because I was trying to figure out where nox came from for the Q mapping. I'm sure whatever drives having [on]unmap also implicitly do sunmap gives a better user experience.

map X xyzzy     [' ']
nunmap X        ['ov']
map X xyzzy     [' ']
xunmap X        ['nos']
map X xyzzy     [' ']
sunmap X        ['nox']
map X xyzzy     [' ']
ounmap X        ['nv']
map X xyzzy     [' ']
sunmap X        ['nox']
smap X xyzzy    ['s', 'nox']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If overloading functions is philosophically undesirable, mapset(null, null, dict) is a possibility.

@brammool
Copy link
Contributor

"maps" isn't commonly used, let's call this "getmappings".

@brammool brammool closed this in 659c240 Apr 24, 2022
@vim-ml
Copy link

vim-ml commented Apr 24, 2022 via email

@errael
Copy link
Contributor Author

errael commented Apr 24, 2022

I'll sit on this for a day, planning on

  • possible name change
  • add optional abbr bool

@errael errael deleted the AddGetmapsBuiltin branch April 24, 2022 18:21
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Nov 9, 2023
…pping

Problem:    Not simple programmatic way to find a specific mapping.
Solution:   Add getmappings(). (Ernie Rael, closes vim/vim#10273)

vim/vim@659c240

Co-authored-by: Ernie Rael <errael@raelity.com>
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Nov 9, 2023
…pping

Problem:    Not simple programmatic way to find a specific mapping.
Solution:   Add getmappings(). (Ernie Rael, closes vim/vim#10273)

vim/vim@659c240

Co-authored-by: Ernie Rael <errael@raelity.com>
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Nov 9, 2023
…pping

Problem:    Not simple programmatic way to find a specific mapping.
Solution:   Add getmappings(). (Ernie Rael, closes vim/vim#10273)

vim/vim@659c240

Co-authored-by: Ernie Rael <errael@raelity.com>
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