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

Bugfix - ensure keymaps dictionary exists before using it #852

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

mnussbaum
Copy link
Contributor

I realized I might have introduced a bug in #851, I haven't been able to trigger it but I wanted to be safe rather then sorry.

The KeyMap.Add and KeyMaps.Remove functions can't use KeyMap._all to fetch the key map dictionary since they need to modify it, and it appears the dictionary returned by the KeyMap._all function is a copy, and modifying the copy doesn't modify the underlying s:keyMaps variable. However, using the s:keyMaps variable directly is error prone since the variable will be undefined until the very first invocation of KeyMap._all.

This PR invokes and discards the result of KeyMap._all prior to adding or removing any key maps from s:keyMapsdirectly, just to ensure s:keyMaps is defined.

Copy link
Contributor

@lifecrisis lifecrisis left a comment

Choose a reason for hiding this comment

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

Rather than add more boilerplate to ensure that s:keyMaps exists, why not just define it at the top of the script? It has to be a script-local variable anyway, so we might as well define it where everyone can see it immediately.

Note that this would allow us to dispense with any checks for the existence of the s:keyMaps variable.

@PhilRunninger
Copy link
Member

Then we wouldn't need the s:KeyMap._all() function either. Just use the s:keyMaps dictionary instead.

The s:KeyMap._all function isn't necessary if we initialize the
s:keyMaps dictionary at file load time.
@mnussbaum mnussbaum force-pushed the fix_maybe_missing_map branch from 0a72edd to eb048a3 Compare June 12, 2018 20:49
@mnussbaum
Copy link
Contributor Author

Good call, I've amended the commit to just initialize the s:keyMaps dictionary at the top of the file and totally removed the KeyMap._all function

@PhilRunninger PhilRunninger merged commit 26abd33 into preservim:master Jun 12, 2018
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