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

keymap: Use snake_case for consistency #23834

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

0xtimsb
Copy link
Member

@0xtimsb 0xtimsb commented Jan 29, 2025

We are already using snake_case in bunch of places for action properties in keymap, but along with it their are few occurances of cameCase as well as PascalCase.

This PR converts following into snake_case, so that it's consistent across:

  1. String:
"ctrl-k ctrl-left": ["workspace::ActivatePaneInDirection", "Left"], // before
"ctrl-k ctrl-left": ["workspace::ActivatePaneInDirection", "left"], // after
  1. Object Keys:
"shift-b": ["vim::PreviousWordStart", { "ignorePunctuation": true }], // before
"shift-b": ["vim::PreviousWordStart", { "ignore_punctuation": true }], // after
  1. Nested Object Keys:
"i": ["vim::PushOperator", { "Object": { "around": false } }], // before 
"i": ["vim::PushOperator", { "object": { "around": false } }], // after
  1. Object Values:
"shift-z shift-z": ["pane::CloseActiveItem", { "saveIntent": "saveAll" }], // before
"shift-z shift-z": ["pane::CloseActiveItem", { "save_intent": "save_all" }], // after

Release Notes:

  • Breaking Change: Keymap now requires action properties to use snake_case. If you've configured a custom keymap, you might need to update it manually.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 29, 2025
@SomeoneToIgnore
Copy link
Contributor

That is a rabbit hole I had lured you in, sorry for such a large scope.

Looking at this PR growing, I think we need to consider one thing (not needed to be done now, but might be needed before the PR is merged): a keymap migration tool.

We do not collect users' keymaps in any form, so it's not clear how many people will suffer from this — presumably, some amount of Vim users?
But we have an error detection, and can think of one-off keymap.json update (and backing up the previous version) for such case.

This are all ideas though, at this stage, thank you for providing more food for thought.

@0xtimsb
Copy link
Member Author

0xtimsb commented Jan 29, 2025

I think the work for this PR is done. I'll open another PR to show a button in the keymap error dialog, but only if we detect it's a casing error.

The button could say something like "Backup and Migrate Keymap" to make it clear what happened. I think the script for this should be fairly simple - at least, that’s what it seems like for now.

@0xtimsb 0xtimsb marked this pull request as ready for review January 29, 2025 19:50
@mikayla-maki
Copy link
Contributor

mikayla-maki commented Jan 29, 2025

Could we just automatically and silently migrate keymaps, while backing them up? If it's a simple find-and-replace job, it doesn't seem like something we have to bother users with.

@0xtimsb 0xtimsb marked this pull request as draft January 29, 2025 19:53
@SomeoneToIgnore
Copy link
Contributor

2 small concerns against that:

  • there could be errors in current keymaps (no migration is possible then)
  • people may use different Zed versions (e.g. developers, bisecting or checking both Preview, Stable and Dev simultaneously)

IMO, it's worth to indicate that migration had happened and show where the backup file is placed.

@@ -725,6 +725,7 @@ impl PaneAxis {
}

#[derive(Clone, Copy, Debug, Deserialize, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit sad that we have to do this — seems that many our code has such conversions already, but I personally see no harm in having the existing, single words in whatever case they are: this way, it will be less breakage to the keymap.

Not insisting though, as it seems not very uniform and we're breaking things anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've thought about this a lot, and initially, I preferred not to break too many things. However, if we keep single words in camelCase, it will affect other enums that might have data like Object in below case:

"i": ["vim::PushOperator", { "Object": { "around": false } }],

So, again inconsistency with keys. Hence, I made this difficult decision.

@mikayla-maki
Copy link
Contributor

there could be errors in current keymaps (no migration is possible then)

I don't think this is blocking, as find and replace doesn't care about errors.

people may use different Zed versions
IMO, it's worth to indicate that migration had happened and show where the backup file is placed.

I agree on both counts, and as a general rule asking before doing things is polite.

@0xtimsb
Copy link
Member Author

0xtimsb commented Jan 29, 2025

Let me figure if simple find and replace will suffice. I strongly think it might be, just checking if there are no edge case doing so.

@0xtimsb 0xtimsb marked this pull request as ready for review January 30, 2025 16:42
@ConradIrwin
Copy link
Member

If we're going to break these...

For things where there are a small number of options like workspace::ActivatePaneInDirection, I think we should make it workspace::ActivatePaneLeft etc.

I added these actions before I knew that actions with arguments didn't show up in the command palette; and think it'd be better to not use that pattern going forward. Sorry!

Probably similar for the ResizePane ones.

For vim::PushOpeator etc. I could go either way - there's a lot of boilerplate already to create a new operator, and it wouldn't be the end of the world to flatten these out.

@0xtimsb 0xtimsb force-pushed the snake_case_keymaps branch from 8db0848 to e5b6352 Compare January 31, 2025 08:40
@0xtimsb
Copy link
Member Author

0xtimsb commented Jan 31, 2025

@ConradIrwin

Apart from flattening you suggested, I also end up doing same for PushOperator:

      "y": ["vim::PushOperator", "yank"], // from
      "y": "vim::PushYank", // to

and

      "f": ["vim::PushOperator", { "find_forward": { "before": false } }], // from
      "f": ["vim::PushFindForward", { "before": false }], // to

I also wonder, now that we are doing that, we should also:

      "escape": ["vim::SwitchMode", "normal"], // from
      "escape": "vim::SwitchToNormalMode", // to

to follow similar convention. What do you think?

@0xtimsb
Copy link
Member Author

0xtimsb commented Jan 31, 2025

I changed it. Felt sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants