0

This is a follow up question to Cycle through a list of letters in multiple groups where I asked how to cycle a group of letters. I got a bunch of great tips and this is my follow up question.

This is simply asking for a code review of my attempt at solving this problem


Explanation

My code simply allows the user to hit some key combination (temporarily set to <localleader>ln and <localleader>lp) to replace the letter under the cursor with the next letter in the cycle.


Here are some of my thoughts on the current state of my code.

My script flashes the contents of the tr command upon execution to the statusline. I tried to avoid this by using ! (silent) without much success.

I tried to make sure I do not mess with the user's registers, however, this solution seems unnecessarily verbose. Any suggestions for improvements here are more than welcome.

s\<c-r>=tr(@\",'" . a:letters . "','" . a:replacements . "')\<cr>\<esc>" executes a command in normal mode, then immediately go to insert mode using s. Then it switches to invoking an (Ex?) command using <C-r>=. Is this really the best way to replace the letter under my cursor by invoking tr over it? It feels as my code jumps between two many modes.

Initially I had some issues with my code, I tried tr(@\"," . a:letters . "," . a:replacements . ") without much success. I had to add ' around the variables to make it work, why? I thought they already were stored as strings?

Can this code be written cleaner/better? This is the first time I've ever written a function in vimscript. I do not know about any style guides, nor how to lint my code. I choose to do this with vimscript, and I'm not really sure why I did that over lua. Alternative approaches are welcome.

Code

function! TryReplaceLetters(letters, replacements)
    let save_cb = &cb
    let regText = getreg('"', 1, 1)
    try
        execute "normal! s\<c-r>=tr(@\",'" . a:letters . "','" . a:replacements . "')\<cr>\<esc>"
    finally
        let &cb = save_cb
        call setreg('"', regText)
    endtry
endfunction 

let s:lookalikes = "1¹2²3³4⁴5⁵6⁶7⁶8⁸9⁹AẠȦÄÆBḄḂƁßCÇĊƇ©DÐḌḊƊEẸĖ£ÆŒFḞƑGĠƓHḤḢIỊİJȷKḲLḶĿMṂṀⱮNṆṄÑƝOȮØŒPṖƤQỌRṚṘ®SṢṠTṬṪƬUỤƲVṾWẈẆXẊYỴẎ¥ZẒŻaạȧæbḅḃþcċç¢dḍḋɗeẹæœfḟƒgḥḣiị¡jkƙlḷŀmṃṁnñoọȯøœpṗÞrṛṙɼsṣṡ$tṭṫƭvʋwẇxẋyżz"
let s:lookcycles = "¹1²2³3⁴4⁵5⁶6⁶7⁸8⁹9ÆAẠȦÄßBḄḂƁ©CÇĊƇƊDÐḌḊŒEẸĖ£ÆƑFḞƓGĠḢHḤİIỊȷJḲKĿLḶⱮMṂṀƝNṆṄÑŒOȮØƤPṖỌQ®RṚṘṠSṢƬTṬṪƲUỤṾVẆWẈẊX¥YỴẎŻZẒæaạȧþbḅḃ¢cċçɗdḍḋœeẹæƒfḟgḣḥ¡iịjƙkŀlḷṁmṃñnœoọȯøÞpṗɼrṛṙ$sṣṡƭtṭṫʋvẇwẋxżyz"

function! JellyLookaLikes()
    " Cycles through similar looking symbols, clockwise
    return TryReplaceLetters(s:lookalikes, s:lookcycles)
endfunction 

function! JellyLookaLikesPrev()
    " Cycles through similar looking symbols, counter-clockwise
    return TryReplaceLetters(s:lookcycles, s:lookalikes)
endfunction 

nnoremap <silent> <localleader>ln :call JellyLookaLikes()<cr><esc>
nnoremap <silent> <localleader>lp :call JellyLookaLikesPrev()<cr><esc>
4
  • 1
    Please try to stick to one main question per question. A code review is a valid question, though, and I can point out a few things.
    – D. Ben Knoble
    Jul 11 at 16:26
  • 1
    As far as I know, there's no style guides. I've read a lot of code in vimscript, and what I've concluded style-wise is that no one really agrees on a best-practice (... beyond stuff enforced by Vim :p). Use whatever you feel comfortable with, or whatever makes sense to you, and whatever you feel makes sense for your project.
    – Zoe
    Jul 11 at 16:46
  • @D.BenKnoble How would a better approach been? I accepted my last question as it answered my question about cycling letters. This, as you say is more of a code review. I can update the title with that. I was under the impression that a Q&A site preferred one question per question. The bullet points are just my thoughts about the code I wrote / what it would be nice to have addressed in a code review =) Jul 11 at 16:51
  • 1
    Bullets are fine, you just seemed to be asking many qs. As a code review, we can address those bullets as part of a larger review
    – D. Ben Knoble
    Jul 11 at 22:35
1

Style

  • You spell out functions/commands/&c., which I think is wonderful. But you didn't spell out &cb. Prefer &clipboard.
  • It appears you mixed spaces and tabs. Prefer one or the other (I use 2 spaces for vimscript).
  • You're missing abort on your functions.
  • function! is not strictly necessary according to :help E127, but this changed—I believe its prevalence is mostly a historical artifact at this point.

Otherwise, stylistically this looks pretty solid. The only other thing I can think of is to use const if you have a new enough vim, and I think execute printf("…", …) looks nicer than string concatenation.

Code

  • The duplication in s:lookalikes and s:lookcycles is somewhat troublesome. I would prefer to find a way to write the pairs down once and then programmatically derive the required parts.

  • You save and restore the clipboard option, but don't change it. Perhaps you meant to include set clipboard&?

  • The clipboard option is only available "in GUI versions or when the |+xterm_clipboard| is included"—you should test for those if you use it.

  • For registers, you should use getreginfo()—it saves more information correctly, and the result can be given directly to setreg().

  • As written, the quoting nastiness is necessary: tr needs string arguments. If you wrote, for example,

    let source = 'bcd'
    let target = 'abc'
    let a = tr('b', source, target)
    

    then everything is fine. But you're inside an execute concatenating strings. Try

    :echo "tr(@\",'" . source . "','" . target . "')"
    

    to see the command you're building—notice the strings have been concatenated, so the fact that they were string variables isn't seen in the final command. I get

    tr(@",'bcd','abc')
    

    Without the quotes, the command would look for variables bcd and abc.

    But we can avoid jumping through all these hoops. Let's use a simple command to grab the character under the cursor. :normal! yl will do, if you like the register stuff. :help strpart() suggests

    strpart(getline('.'), col('.') - 1, 1, v:true)
    

    to get the character under the cursor (but this might fail for multi-byte characters, such as emoji).

    Either way, we get the character in a variable we can manipulate normally, and then finally do an :execute ':normal! r'.replacement.

Putting it altogether, I would write something like below. I've left the letters alone because, to be honest, it was very difficult to read.

function TryReplaceLetters(letters, replacements) abort
  if has('gui') || has('xterm_clipboard')
    let save_cb = &clipboard
    set clipboard&
  endif
  let save_reg = getreginfo('"')
  try
    normal! yl
    let char_to_change = @"
  finally
    if has('gui') || has('xterm_clipboard')
      let &clipboard = save_cb
    endif
    call setreg('"', save_reg)
  endtry
  let replacement = tr(char_to_change, a:letters, a:replacements)
  " avoid changes if same character
  if replacement isnot# char_to_change
    execute ':normal! r'.replacement
  endif
endfunction

let s:lookalikes = "1¹2²3³4⁴5⁵6⁶7⁶8⁸9⁹AẠȦÄÆBḄḂƁßCÇĊƇ©DÐḌḊƊEẸĖ£ÆŒFḞƑGĠƓHḤḢIỊİJȷKḲLḶĿMṂṀⱮNṆṄÑƝOȮØŒPṖƤQỌRṚṘ®SṢṠTṬṪƬUỤƲVṾWẈẆXẊYỴẎ¥ZẒŻaạȧæbḅḃþcċç¢dḍḋɗeẹæœfḟƒgḥḣiị¡jkƙlḷŀmṃṁnñoọȯøœpṗÞrṛṙɼsṣṡ$tṭṫƭvʋwẇxẋyżz"
let s:lookcycles = "¹1²2³3⁴4⁵5⁶6⁶7⁸8⁹9ÆAẠȦÄßBḄḂƁ©CÇĊƇƊDÐḌḊŒEẸĖ£ÆƑFḞƓGĠḢHḤİIỊȷJḲKĿLḶⱮMṂṀƝNṆṄÑŒOȮØƤPṖỌQ®RṚṘṠSṢƬTṬṪƲUỤṾVẆWẈẊX¥YỴẎŻZẒæaạȧþbḅḃ¢cċçɗdḍḋœeẹæƒfḟgḣḥ¡iịjƙkŀlḷṁmṃñnœoọȯøÞpṗɼrṛṙ$sṣṡƭtṭṫʋvẇwẋxżyz"

function JellyLookaLikes() abort
  " Cycles through similar looking symbols, clockwise
  return TryReplaceLetters(s:lookalikes, s:lookcycles)
endfunction

function JellyLookaLikesPrev() abort
  " Cycles through similar looking symbols, counter-clockwise
  return TryReplaceLetters(s:lookcycles, s:lookalikes)
endfunction

nnoremap <silent> <localleader>ln :call JellyLookaLikes()<cr><esc>
nnoremap <silent> <localleader>lp :call JellyLookaLikesPrev()<cr><esc>

Here's an example of turning a list of cycles into the two arguments necessary: assume each cycle is a list of characters, and cycles is a list of cycles.

function ToTrArgs(cycles) abort
  let first_argument = deepcopy(a:cycles)->flatten()->join('')
  let second_argument = mapnew(a:cycles, {_, v -> [v[-1]]+v[:-2]})->flatten()->join('')
  return [first_argument, second_argument]
endfunction

let [s:forwards, s:backwards] = ToTrArgs([['a','á','à'], ['b', 'ḃ']])

function JellyLookaLikes()
  " Cycles through similar looking symbols, clockwise
  return TryReplaceLetters(s:forwards, s:backwards)
endfunction

function JellyLookaLikesPrev()
  " Cycles through similar looking symbols, counter-clockwise
  return TryReplaceLetters(s:backwards, s:forwards)
endfunction

nnoremap <silent> <localleader>ln :call JellyLookaLikes()<CR>
nnoremap <silent> <localleader>lp :call JellyLookaLikesPrev()<CR>
" alternatively, if you don't like the whole "s:var not valid in mapping" thing,
" use call() with a list argument
nnoremap <silent> <localleader>ln :call call('TryReplaceLetters', ToTrArgs([['a','á','à'], ['b', 'ḃ']]))<CR>
nnoremap <silent> <localleader>lp :call call('TryReplaceLetters', ToTrArgs([['a','á','à'], ['b', 'ḃ']])->reverse())<CR>
2
  • fantastic review! I will read it throughly when I have time. Just to make sure as long as I am only working on the @ register I do not need any of the code related to the clipboard right? I just wanted to make sure I do not mess up anything. I was unsure of my uses of l:, a: to narrow the scope of my functions, and I see you do not use these either. Are these also a relic form the past? And again thanks a bunch Jul 12 at 17:05
  • 1
    Honestly I’m not sure if clipboard needs modified. It might. I don’t use l: anymore inside functions, but I use a: (mandatory), s:, g:, w:, t:, b:, etc.
    – D. Ben Knoble
    Jul 12 at 17:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.