vim-erlang / vim-erlang-runtime

Erlang indentation and syntax for Vim
https://vim-erlang.github.io
101 stars 29 forks source link

Refactor ftplugin #48

Closed slarwise closed 3 years ago

slarwise commented 3 years ago

Refactor the ftplugin script.

Simpler and more readable in my opinion, and fixes some bugs with b:undo_ftplugin and cpoptions. Also added tests to make sure all options/variables are set correctly =)

See the commit message of b4b0a2d for details of all the changes. A bit opinionated of course, so feel free to disagree with stuff 🙃

hcs42 commented 3 years ago
  1. I like the refactoring.

    Could you add yourself to the "Contributor" list in the beginning the script?

  2. The new tests fail on my machine:

    Starting Vader: 1 suite(s), 4 case(s)
      Starting Vader: /home/hcs/h/hcs-cp/vim-erlang/vim-erlang-runtime/test_ftplugin_set_options.vader
        (1/4) [EXECUTE] Don't set any g:erlang_* variables
        (1/4) [  AFTER] (X) 'aABceFs' should be equal to 'aABceFs_'
        (2/4) [EXECUTE] Set g:erlang_keywordprg
        (2/4) [  AFTER] (X) 'aABceFs' should be equal to 'aABceFs_'
        (3/4) [EXECUTE] Set g:erlang_folding
        (3/4) [  AFTER] (X) 'aABceFs' should be equal to 'aABceFs_'
        (4/4) [EXECUTE] Set g:erlang_keywordprg and g:erlang_folding
        (4/4) [  AFTER] (X) 'aABceFs' should be equal to 'aABceFs_'
      Success/Total: 0/4
    Success/Total: 0/4 (assertions: 4/8)
    Elapsed time: 0.36 sec.

    :help for me says that the default value for cpo is "aABceFs", and it doesn't mention _ as a possible flag.

    I also looked at the online Vim documentation, which is "For Vim version 8.2. Last change: 2020 Oct 10", and it also doesn't mention the _ flag.

    Do you have such a flag in your Vim version?

  3. I pushed a commit to the master branch where I moved the earlier test files into the "test" directory. They have a better place there. Could you move the vader tests there too?

slarwise commented 3 years ago
  1. Done.

  2. Ah, good catch! I tested it on Neovim, and they actually have a different default value for cpoptions. I changed the options that are not expected to change from the nvim/vim defaults from being hardcoded to instead use the value before the ftplugin script is sourced. So this should work with whatever the default value is for the vim version. Have tested it on Vim 8.2 and Neovim 0.5.

  3. Done.

hcs42 commented 3 years ago

In the Move vader test files to "test" directory commit, you need to add a .. to the "source" lines:

-source ftplugin/erlang.vim
+source ../ftplugin/erlang.vim

Otherwise the tests fail with Can't open file ftplugin/erlang.vim.

Could you amend the commit accordingly?

Everything else in the PR looks good.

slarwise commented 3 years ago

Nice thanks, should be good now 👍 Updated README on how to run vader tests as well to match this.