Always expand all paths (journals, templates, etc) (#1468)

* Refactored path expansion calls into a new path.py file

This also fixed bugs with relative journal and template paths.

* Update tests for new path functions

Also, make the tests specific to Windows, Mac & Linux

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>
Co-authored-by: Micah Jerome Ellison <micah.jerome.ellison@gmail.com>

jrnl/Journal.py

@@ -11,6 +11,7 @@ import sys
 from . import Entry
 from . import time
 from .prompt import yesno
+from .path import expand_path
 
 
 class Tag:
@@ -410,7 +411,7 @@ def open_journal(journal_name, config, legacy=False):
     backwards compatibility with jrnl 1.x
     """
     config = config.copy()
-    config["journal"] = os.path.expanduser(os.path.expandvars(config["journal"]))
+    config["journal"] = expand_path(config["journal"])
 
     if os.path.isdir(config["journal"]):
         if config["encrypt"]:

jrnl/config.py

@@ -15,6 +15,7 @@ from jrnl.messages import MsgType
 from .color import ERROR_COLOR
 from .color import RESET_COLOR
 from .output import list_journals
+from .path import home_dir
 
 # Constants
 DEFAULT_CONFIG_NAME = "jrnl.yaml"
@@ -83,9 +84,7 @@ def get_config_path():
             ),
         )
 
-    return os.path.join(
+    return os.path.join(config_directory_path or home_dir(), DEFAULT_CONFIG_NAME)
-        config_directory_path or os.path.expanduser("~"), DEFAULT_CONFIG_NAME
-    )
 
 
 def get_default_config():
@@ -112,9 +111,7 @@ def get_default_config():
 
 
 def get_default_journal_path():
-    journal_data_path = xdg.BaseDirectory.save_data_path(
+    journal_data_path = xdg.BaseDirectory.save_data_path(XDG_RESOURCE) or home_dir()
-        XDG_RESOURCE
-    ) or os.path.expanduser("~")
     return os.path.join(journal_data_path, DEFAULT_JOURNAL_NAME)
 
 

jrnl/install.py

@@ -7,6 +7,9 @@ import logging
 import os
 import sys
 
+from .path import home_dir
+from .path import absolute_path
+from .path import expand_path
 from .config import DEFAULT_JOURNAL_KEY
 from .config import get_config_path
 from .config import get_default_config
@@ -45,7 +48,7 @@ def find_default_config():
     config_path = (
         get_config_path()
         if os.path.exists(get_config_path())
-        else os.path.join(os.path.expanduser("~"), ".jrnl_config")
+        else os.path.join(home_dir(), ".jrnl_config")
     )
     return config_path
 
@@ -101,11 +104,9 @@ def install():
     # Where to create the journal?
     default_journal_path = get_default_journal_path()
     path_query = f"Path to your journal file (leave blank for {default_journal_path}): "
-    journal_path = os.path.abspath(input(path_query).strip() or default_journal_path)
+    journal_path = absolute_path(input(path_query).strip() or default_journal_path)
     default_config = get_default_config()
-    default_config["journals"][DEFAULT_JOURNAL_KEY] = os.path.expanduser(
+    default_config["journals"][DEFAULT_JOURNAL_KEY] = journal_path
-        os.path.expandvars(journal_path)
-    )
 
     # If the folder doesn't exist, create it
     path = os.path.split(default_config["journals"][DEFAULT_JOURNAL_KEY])[0]
@@ -138,7 +139,7 @@ def _initialize_autocomplete():
 
 
 def _autocomplete_path(text, state):
-    expansions = glob.glob(os.path.expanduser(os.path.expandvars(text)) + "*")
+    expansions = glob.glob(expand_path(text) + "*")
     expansions = [e + "/" if os.path.isdir(e) else e for e in expansions]
     expansions.append(None)
     return expansions[state]

jrnl/jrnl.py

@@ -14,6 +14,7 @@ from .editor import get_text_from_editor
 from .editor import get_text_from_stdin
 from . import time
 from .override import apply_overrides
+from .path import expand_path
 
 from jrnl.exception import JrnlException
 from jrnl.messages import Message
@@ -217,8 +218,10 @@ def _get_editor_template(config, **kwargs):
         logging.debug("Write mode: no template configured")
         return ""
 
+    template_path = expand_path(config["template"])
+
     try:
-        template = open(config["template"]).read()
+        template = open(template_path).read()
         logging.debug("Write mode: template loaded: %s", template)
     except OSError:
         logging.error("Write mode: template not loaded")
@@ -226,7 +229,7 @@ def _get_editor_template(config, **kwargs):
             Message(
                 MsgText.CantReadTemplate,
                 MsgType.ERROR,
-                {"template": config["template"]},
+                {"template": template_path},
             )
         )
 

jrnl/path.py

@@ -0,0 +1,13 @@
+import os.path
+
+
+def home_dir():
+    return os.path.expanduser("~")
+
+
+def expand_path(path):
+    return os.path.expanduser(os.path.expandvars(path))
+
+
+def absolute_path(path):
+    return os.path.abspath(expand_path(path))

jrnl/upgrade.py

@@ -11,6 +11,7 @@ from .config import is_config_json
 from .config import load_config
 from .config import scope_config
 from .prompt import yesno
+from .path import expand_path
 
 from jrnl.output import print_msg
 
@@ -22,7 +23,7 @@ from jrnl.messages import MsgType
 
 def backup(filename, binary=False):
     print(f"  Created a backup at {filename}.backup", file=sys.stderr)
-    filename = os.path.expanduser(os.path.expandvars(filename))
+    filename = expand_path(filename)
 
     try:
         with open(filename, "rb" if binary else "r") as original:
@@ -72,15 +73,13 @@ older versions of jrnl anymore.
 
     for journal_name, journal_conf in config["journals"].items():
         if isinstance(journal_conf, dict):
-            path = journal_conf.get("journal")
+            path = expand_path(journal_conf.get("journal"))
             encrypt = journal_conf.get("encrypt")
         else:
             encrypt = config.get("encrypt")
-            path = journal_conf
+            path = expand_path(journal_conf)
 
-        if os.path.exists(os.path.expanduser(path)):
+        if not os.path.exists(path):
-            path = os.path.expanduser(path)
-        else:
             print(f"\nError: {path} does not exist.")
             continue
 

pyproject.toml

@@ -81,6 +81,10 @@ required_plugins = [
 ]
 markers = [
     "todo",
+    "skip_win",
+    "skip_posix",
+    "on_win",
+    "on_posix",
 ]
 addopts = [
   "--pdbcls=IPython.terminal.debugger:Pdb"

tests/conftest.py

@@ -2,8 +2,10 @@
 # License: https://www.gnu.org/licenses/gpl-3.0.html
 
 from pytest import mark
+from pytest import skip
 
 from jrnl.os_compat import on_windows
+from jrnl.os_compat import on_posix
 
 
 pytest_plugins = [
@@ -15,11 +17,39 @@ pytest_plugins = [
 
 
 def pytest_bdd_apply_tag(tag, function):
+    # skip markers
     if tag == "skip_win":
         marker = mark.skipif(on_windows(), reason="Skip test on Windows")
+    elif tag == "skip_posix":
+        marker = mark.skipif(on_posix(), reason="Skip test on Mac/Linux")
+
+    # only on OS markers
+    elif tag == "on_win":
+        marker = mark.skipif(not on_windows(), reason="Skip test not on Windows")
+    elif tag == "on_posix":
+        marker = mark.skipif(not on_posix(), reason="Skip test not on Mac/Linux")
     else:
         # Fall back to pytest-bdd's default behavior
         return None
 
     marker(function)
     return True
+
+
+def pytest_runtest_setup(item):
+    markers = [mark.name for mark in item.iter_markers()]
+
+    on_win = on_windows()
+    on_nix = on_posix()
+
+    if "skip_win" in markers and on_win:
+        skip("Skip test on Windows")
+
+    if "skip_posix" in markers and on_nix:
+        skip("Skip test on Mac/Linux")
+
+    if "on_win" in markers and not on_win:
+        skip("Skip test not on Windows")
+
+    if "on_posix" in markers and not on_nix:
+        skip("Skip test not on Mac/Linux")

tests/unit/test_path.py

@@ -0,0 +1,103 @@
+import pytest
+import random
+import string
+
+from os import getenv
+from unittest.mock import patch
+
+from jrnl.path import home_dir
+from jrnl.path import expand_path
+from jrnl.path import absolute_path
+
+
+@pytest.fixture
+def home_dir_str(monkeypatch):
+    username = "username"
+    monkeypatch.setenv("USERPROFILE", username)  # for windows
+    monkeypatch.setenv("HOME", username)  # for *nix
+    return username
+
+
+@pytest.fixture
+def random_test_var(monkeypatch):
+    name = f"JRNL_TEST_{''.join(random.sample(string.ascii_uppercase, 10))}"
+    val = "".join(random.sample(string.ascii_lowercase, 25))
+    monkeypatch.setenv(name, val)
+    return (name, val)
+
+
+def test_home_dir(home_dir_str):
+    assert home_dir() == home_dir_str
+
+
+@pytest.mark.on_posix
+@pytest.mark.parametrize(
+    "path",
+    ["~"],
+)
+def test_expand_path_actually_expands_mac_linux(path):
+    # makes sure that path isn't being returns as-is
+    assert expand_path(path) != path
+
+
+@pytest.mark.on_win
+@pytest.mark.parametrize(
+    "path",
+    ["~", "%USERPROFILE%"],
+)
+def test_expand_path_actually_expands_windows(path):
+    # makes sure that path isn't being returns as-is
+    assert expand_path(path) != path
+
+
+@pytest.mark.on_posix
+@pytest.mark.parametrize(
+    "paths",
+    [
+        ["~", "HOME"],
+    ],
+)
+def test_expand_path_expands_into_correct_value_mac_linux(paths):
+    input_path, expected_path = paths[0], paths[1]
+    assert expand_path(input_path) == getenv(expected_path)
+
+
+@pytest.mark.on_win
+@pytest.mark.parametrize(
+    "paths",
+    [
+        ["~", "USERPROFILE"],
+        ["%USERPROFILE%", "USERPROFILE"],
+    ],
+)
+def test_expand_path_expands_into_correct_value_windows(paths):
+    input_path, expected_path = paths[0], paths[1]
+    assert expand_path(input_path) == getenv(expected_path)
+
+
+@pytest.mark.on_posix
+@pytest.mark.parametrize("_", range(25))
+def test_expand_path_expands_into_random_env_value_mac_linux(_, random_test_var):
+    var_name, var_value = random_test_var[0], random_test_var[1]
+    assert expand_path(var_name) == var_name
+    assert expand_path(f"${var_name}") == var_value  # mac & linux
+    assert expand_path(f"${var_name}") == getenv(var_name)
+
+
+@pytest.mark.on_win
+@pytest.mark.parametrize("_", range(25))
+def test_expand_path_expands_into_random_env_value_windows(_, random_test_var):
+    var_name, var_value = random_test_var[0], random_test_var[1]
+    assert expand_path(var_name) == var_name
+    assert expand_path(f"%{var_name}%") == var_value  # windows
+    assert expand_path(f"%{var_name}%") == getenv(var_name)
+
+
+@patch("jrnl.path.expand_path")
+@patch("os.path.abspath")
+def test_absolute_path(mock_abspath, mock_expand_path):
+    test_val = "test_value"
+
+    assert absolute_path(test_val) == mock_abspath.return_value
+    mock_expand_path.assert_called_with(test_val)
+    mock_abspath.assert_called_with(mock_expand_path.return_value)