Vim plugin fix string normalization option

[btel]

This PR fixes parsing of the skip-string-normalization option in vim plugin. Originally, the plugin read the string-normalization option, which does not exist in help (--help) and it's not respected by black on command line.

[ichard26]

Checking in, @btel is this PR still relevant? Thank you for submitting it by the way!

[godlygeek]

I think this PR is still relevant, though I believe it can be simplified.

It seems to be fixing two entirely different problems related to how pyproject.toml files are handled by the vim plugin.

1. The plugin fails to properly read boolean values from pyproject.toml. For instance, if you create this pyproject.toml:

[tool.black]
quiet = true

the Black CLI is happy with it and runs without any messages, but the :Black command provided by this plugin fails with:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "<string>", line 102, in Black
  File "<string>", line 150, in get_configs
  File "<string>", line 150, in <dictcomp>
  File "/usr/lib/python3.6/distutils/util.py", line 311, in strtobool
    val = val.lower()
AttributeError: 'bool' object has no attribute 'lower'

That's because the value returned by the toml.load() is already a bool, but the vim plugin incorrectly tries to convert it from a str to a bool.

2. The vim plugin fails to take the value for skip_string_normalization from pyproject.toml. That's because it looks for a string_normalization key instead of a skip_string_normalization key, thanks to this line saying the name of the flag is string_normalization:

   black/autoload/black.vim

   Line 25 in 05b54b8

   Flag(name="string_normalization", cast=strtobool),

   
   and this dictcomp looking up each flag's name in the config dict:

   black/autoload/black.vim

   Lines 148 to 151 in 05b54b8

   	return {
   	flag.var_name: flag.cast(toml_config.get(flag.name, vim.eval(flag.vim_rc_name)))
   	for flag in FLAGS
   	}

[godlygeek]

For the first issue, this patch is sufficient:

diff --git a/autoload/black.vim b/autoload/black.vim
index f0357b0..268490e 100644
--- a/autoload/black.vim
+++ b/autoload/black.vim
@@ -146,7 +146,7 @@ def get_configs():
     toml_config = {}
 
   return {
-    flag.var_name: flag.cast(toml_config.get(flag.name, vim.eval(flag.vim_rc_name)))
+    flag.var_name: toml_config.get(flag.name, flag.cast(vim.eval(flag.vim_rc_name)))
     for flag in FLAGS
   }
 

The value returned by toml_config.get() was always being passed to flag.cast(), which is a function that either converts a string to an int or a string to a bool, depending on the flag. vim.eval() returns integers and strings all as str, which is why we need the cast, but that's the wrong thing to do for values that came from toml.load(). We should be applying the cast only to the return from vim.eval() (since we know it always gives us a string), rather than casting the value that toml.load() found - which is already the right type.

[godlygeek]

For the second issue, I think I'd do a slightly different patch. I'd keep the change to invert this flag's meaning and change its name that this PR proposes, but I'd also change the handling of the g:black_skip_string_normalization and g:black_string_normalization variables to make it clear that g:black_skip_string_normalization is the expected name, and g:black_string_normalization is only checked when the expected name is unset, for backwards compatibility.

diff --git a/autoload/black.vim b/autoload/black.vim
index f0357b0..7698829 100644
--- a/autoload/black.vim
+++ b/autoload/black.vim
@@ -22,7 +22,7 @@ class Flag(collections.namedtuple("FlagBase", "name, cast")):
 FLAGS = [
   Flag(name="line_length", cast=int),
   Flag(name="fast", cast=strtobool),
-  Flag(name="string_normalization", cast=strtobool),
+  Flag(name="skip_string_normalization", cast=strtobool),
   Flag(name="quiet", cast=strtobool),
 ]
 
@@ -103,7 +103,7 @@ def Black():
   configs = get_configs()
   mode = black.FileMode(
     line_length=configs["line_length"],
-    string_normalization=configs["string_normalization"],
+    string_normalization=not configs["skip_string_normalization"],
     is_pyi=vim.current.buffer.name.endswith('.pyi'),
   )
   quiet = configs["quiet"]
diff --git a/plugin/black.vim b/plugin/black.vim
index b5edb2a..a5a082f 100644
--- a/plugin/black.vim
+++ b/plugin/black.vim
@@ -43,11 +43,11 @@ endif
 if !exists("g:black_linelength")
   let g:black_linelength = 88
 endif
-if !exists("g:black_string_normalization")
-  if exists("g:black_skip_string_normalization")
-    let g:black_string_normalization = !g:black_skip_string_normalization
+if !exists("g:black_skip_string_normalization")
+  if exists("g:black_string_normalization")
+    let g:black_skip_string_normalization = !g:black_string_normalization
   else
-    let g:black_string_normalization = 1
+    let g:black_skip_string_normalization = 0
   endif
 endif
 if !exists("g:black_quiet")

For those who can't read vimscript, the let statement assigns a variable, the g: prefix on all the variables indicates that they're globals, and the exists function returns true if a variable is defined.

The old behavior was to check if g:black_string_normalization is defined and to define it if not, using the inverse of g:black_skip_string_normalization if that is set, and otherwise to the default of 1.

My proposed behavior is to check if g:black_skip_string_normalization is defined and to define it if not, using the inverse of g:black_string_normalization if that is set, and otherwise to the default of 0. The Python code in autoload/black.vim runs later, and will use the value of g:black_skip_string_normalization (and ignore g:black_string_normalization; it will only be used to set g:black_skip_string_normalization if it wasn't already set).

[godlygeek]

LGTM