Fix OS compatibility issues for editors with spaces, slashes, and quotes (#1153)

* Fix inverted POSIX check, refactor os_compat, and add tests for it
* Fix missing parentheses and remove skip_win on test that is passing in Windows now
* Fix expected quotes in quoted args
* Make  output clearer on failing test
* Bringing skip_win back to test whose failure is a bit more complicated than expected

7 files changed, 121 insertions, 20 deletions

diff --git a/features/environment.py b/features/environment.py
index 71ed8969..f4baab34 100644
--- a/features/environment.py
+++ b/features/environment.py
@@ -42,7 +42,7 @@ def before_feature(context, feature):
         feature.skip()
         return
 
-    if "skip_win" in feature.tags and on_windows:
+    if "skip_win" in feature.tags and on_windows():
         feature.skip("Skipping on Windows")
         return
 
@@ -69,7 +69,7 @@ def before_scenario(context, scenario):
         scenario.skip()
         return
 
-    if "skip_win" in scenario.effective_tags and on_windows:
+    if "skip_win" in scenario.effective_tags and on_windows():
         scenario.skip("Skipping on Windows")
         return
 
diff --git a/features/steps/core.py b/features/steps/core.py
index f5215d58..abac4917 100644
--- a/features/steps/core.py
+++ b/features/steps/core.py
@@ -6,7 +6,6 @@ from collections import defaultdict
 import os
 from pathlib import Path
 import re
-import shlex
 import time
 from unittest.mock import patch
 
@@ -23,7 +22,7 @@ from jrnl import __version__
 from jrnl import plugins
 from jrnl.cli import cli
 from jrnl.config import load_config
-from jrnl.os_compat import on_windows
+from jrnl.os_compat import split_args
 
 try:
     import parsedatetime.parsedatetime_consts as pdt
@@ -88,10 +87,6 @@ class FailedKeyring(keyring.backend.KeyringBackend):
 keyring.set_keyring(TestKeyring())
 
 
-def ushlex(command):
-    return shlex.split(command, posix=not on_windows)
-
-
 def read_journal(context, journal_name="default"):
     configuration = load_config(context.config_path)
     with open(configuration["journals"][journal_name]) as journal_file:
@@ -319,7 +314,7 @@ def run_with_input(context, command, inputs=""):
     else:
         text = iter([inputs])
 
-    args = ushlex(command)[1:]
+    args = split_args(command)[1:]
 
     def _mock_editor(command):
         context.editor_command = command
@@ -403,7 +398,7 @@ def run(context, command, text=""):
         cache_dir = os.path.join("features", "cache", context.cache_dir)
         command = command.format(cache_dir=cache_dir)
 
-    args = ushlex(command)
+    args = split_args(command)
 
     def _mock_editor(command):
         context.editor_command = command
diff --git a/features/steps/export_steps.py b/features/steps/export_steps.py
index 6a5c8e46..f885591c 100644
--- a/features/steps/export_steps.py
+++ b/features/steps/export_steps.py
@@ -169,17 +169,24 @@ def assert_exported_yaml_file_content(context, file_path, cache_dir=None):
 
     for actual_line, expected_line in zip(actual_content, expected_content):
         if actual_line.startswith("tags: ") and expected_line.startswith("tags: "):
-            assert_equal_tags_ignoring_order(actual_line, expected_line)
+            assert_equal_tags_ignoring_order(
+                actual_line, expected_line, actual_content, expected_content
+            )
         else:
             assert actual_line.strip() == expected_line.strip(), [
-                actual_line.strip(),
-                expected_line.strip(),
+                [actual_line.strip(), expected_line.strip()],
+                [actual_content, expected_content],
             ]
 
 
-def assert_equal_tags_ignoring_order(actual_line, expected_line):
+def assert_equal_tags_ignoring_order(
+    actual_line, expected_line, actual_content, expected_content
+):
     actual_tags = set(tag.strip() for tag in actual_line[len("tags: ") :].split(","))
     expected_tags = set(
         tag.strip() for tag in expected_line[len("tags: ") :].split(",")
     )
-    assert actual_tags == expected_tags, [actual_tags, expected_tags]
+    assert actual_tags == expected_tags, [
+        [actual_tags, expected_tags],
+        [expected_content, actual_content],
+    ]
diff --git a/jrnl/color.py b/jrnl/color.py
index dca28117..3bdd4149 100644
--- a/jrnl/color.py
+++ b/jrnl/color.py
@@ -7,7 +7,7 @@ import colorama
 
 from .os_compat import on_windows
 
-if on_windows:
+if on_windows():
     colorama.init()
 
 WARNING_COLOR = colorama.Fore.YELLOW
diff --git a/jrnl/editor.py b/jrnl/editor.py
index 1a68028d..086d84db 100644
--- a/jrnl/editor.py
+++ b/jrnl/editor.py
@@ -1,6 +1,5 @@
 import logging
 import os
-import shlex
 import subprocess
 import sys
 import tempfile
@@ -10,6 +9,7 @@ from pathlib import Path
 from .color import ERROR_COLOR
 from .color import RESET_COLOR
 from .os_compat import on_windows
+from .os_compat import split_args
 
 
 def get_text_from_editor(config, template=""):
@@ -25,7 +25,7 @@ def get_text_from_editor(config, template=""):
             f.write(template)
 
     try:
-        subprocess.call(shlex.split(config["editor"], posix=on_windows) + [tmpfile])
+        subprocess.call(split_args(config["editor"]) + [tmpfile])
     except Exception as e:
         error_msg = f"""
         {ERROR_COLOR}{str(e)}{RESET_COLOR}
@@ -47,7 +47,7 @@ def get_text_from_editor(config, template=""):
 
 
 def get_text_from_stdin():
-    _how_to_quit = "Ctrl+z and then Enter" if on_windows else "Ctrl+d"
+    _how_to_quit = "Ctrl+z and then Enter" if on_windows() else "Ctrl+d"
     print(
         f"[Writing Entry; on a blank line, press {_how_to_quit} to finish writing]\n",
         file=sys.stderr,
diff --git a/jrnl/os_compat.py b/jrnl/os_compat.py
index b38d9d60..6615b886 100644
--- a/jrnl/os_compat.py
+++ b/jrnl/os_compat.py
@@ -1,6 +1,18 @@
 # Copyright (C) 2012-2021 jrnl contributors
 # License: https://www.gnu.org/licenses/gpl-3.0.html
 
+import shlex
 from sys import platform
 
-on_windows = "win32" in platform
+
+def on_windows():
+    return "win32" in platform
+
+
+def on_posix():
+    return not on_windows()
+
+
+def split_args(args):
+    """Split arguments and add escape characters as appropriate for the OS"""
+    return shlex.split(args, posix=on_posix())
diff --git a/tests/test_os_compat.py b/tests/test_os_compat.py
new file mode 100644
index 00000000..f7c058f1
--- /dev/null
+++ b/tests/test_os_compat.py
@@ -0,0 +1,87 @@
+from unittest import mock
+import pytest
+
+from jrnl.os_compat import on_windows
+from jrnl.os_compat import on_posix
+from jrnl.os_compat import split_args
+
+
+@pytest.mark.parametrize(
+    "systems",
+    [
+        ["linux", False],
+        ["win32", True],
+        ["cygwin", False],
+        ["msys", False],
+        ["darwin", False],
+        ["os2", False],
+        ["os2emx", False],
+        ["riscos", False],
+        ["atheos", False],
+        ["freebsd7", False],
+        ["freebsd8", False],
+        ["freebsdN", False],
+        ["openbsd6", False],
+    ],
+)
+def test_on_windows(systems):
+    osname, expected_on_windows = systems[0], systems[1]
+    with mock.patch("jrnl.os_compat.platform", osname):
+        assert on_windows() == expected_on_windows
+
+
+@pytest.mark.parametrize(
+    "systems",
+    [
+        ["linux", True],
+        ["win32", False],
+        ["cygwin", True],
+        ["msys", True],
+        ["darwin", True],
+        ["os2", True],
+        ["os2emx", True],
+        ["riscos", True],
+        ["atheos", True],
+        ["freebsd7", True],
+        ["freebsd8", True],
+        ["freebsdN", True],
+        ["openbsd6", True],
+    ],
+)
+def test_on_posix(systems):
+    osname, expected_on_posix = systems[0], systems[1]
+    with mock.patch("jrnl.os_compat.platform", osname):
+        assert on_posix() == expected_on_posix
+
+
+@pytest.mark.parametrize(
+    "args",
+    [
+        ["notepad", ["notepad"]],
+        ["subl -w", ["subl", "-w"]],
+        [
+            '"C:\\Program Files\\Sublime Text 3\\subl.exe" -w',
+            ['"C:\\Program Files\\Sublime Text 3\\subl.exe"', "-w"],
+        ],
+    ],
+)
+def test_split_args_on_windows(args):
+    input_arguments, expected_split_args = args[0], args[1]
+    with mock.patch("jrnl.os_compat.on_windows", lambda: True):
+        assert split_args(input_arguments) == expected_split_args
+
+
+@pytest.mark.parametrize(
+    "args",
+    [
+        ["vim", ["vim"]],
+        [
+            'vim -f +Goyo +Limelight "+set spell linebreak"',
+            ["vim", "-f", "+Goyo", "+Limelight", '"+set spell linebreak"'],
+        ],
+    ],
+)
+def test_split_args_on_not_windows(args):
+    input_arguments, expected_split_args = args[0], args[1]
+    with mock.patch("jrnl.os_compat.on_windows", lambda: True):
+        assert split_args(input_arguments) == expected_split_args