From 9e6cd8820f3e7ff426bafd27d9ae955806404425 Mon Sep 17 00:00:00 2001 From: Micah Jerome Ellison Date: Sat, 16 Jan 2021 15:19:11 -0800 Subject: [PATCH] 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 --- features/environment.py | 4 +- features/steps/core.py | 11 ++--- features/steps/export_steps.py | 17 +++++-- jrnl/color.py | 2 +- jrnl/editor.py | 6 +-- jrnl/os_compat.py | 14 +++++- tests/test_os_compat.py | 87 ++++++++++++++++++++++++++++++++++ 7 files changed, 121 insertions(+), 20 deletions(-) create mode 100644 tests/test_os_compat.py 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