Refactor --template code (#1711)

* Move path concerns to path.py and template concerns to editor.py -- BDD tests are failing
* Move path-related constants from config.py to path.py
* Mock get_templates_path in its new calling file
* Mediate template arg vs. config in controller then read template text in editor and unify those two use cases.
Some tests still failing
* Fix test whose message had changed
* poe format
* Refactor for easier unit testing and add unit tests
* Use path strings instead of Path objects in return values to prevent side effects that caused unit tests to fail on some platforms
* poe format
* Attempt to bypass getcwd errors in CI with patch
* Consistently use strings for paths instead of some strings and some pathlib.Path
* Keep pathlib within a function for readability
* fix for ruamel.yaml versions >=0.17.22
* Run poe format

---------

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>

jrnl/config.py

@@ -4,12 +4,10 @@
 import argparse
 import logging
 import os
-from pathlib import Path
 from typing import Any
 from typing import Callable
 
 import colorama
-import xdg.BaseDirectory
 from rich.pretty import pretty_repr
 from ruamel.yaml import YAML
 from ruamel.yaml import constructor
@@ -21,13 +19,10 @@ from jrnl.messages import MsgStyle
 from jrnl.messages import MsgText
 from jrnl.output import list_journals
 from jrnl.output import print_msg
-from jrnl.path import home_dir
+from jrnl.path import get_config_path
+from jrnl.path import get_default_journal_path
 
 # Constants
-DEFAULT_CONFIG_NAME = "jrnl.yaml"
-XDG_RESOURCE = "jrnl"
-
-DEFAULT_JOURNAL_NAME = "journal.txt"
 DEFAULT_JOURNAL_KEY = "default"
 
 YAML_SEPARATOR = ": "
@@ -73,31 +68,6 @@ def save_config(config: dict, alt_config_path: str | None = None) -> None:
         yaml.dump(config, f)
 
 
-def get_config_directory() -> str:
-    try:
-        return xdg.BaseDirectory.save_config_path(XDG_RESOURCE)
-    except FileExistsError:
-        raise JrnlException(
-            Message(
-                MsgText.ConfigDirectoryIsFile,
-                MsgStyle.ERROR,
-                {
-                    "config_directory_path": os.path.join(
-                        xdg.BaseDirectory.xdg_config_home, XDG_RESOURCE
-                    )
-                },
-            ),
-        )
-
-
-def get_config_path() -> Path:
-    try:
-        config_directory_path = get_config_directory()
-    except JrnlException:
-        return Path(home_dir(), DEFAULT_CONFIG_NAME)
-    return Path(config_directory_path, DEFAULT_CONFIG_NAME)
-
-
 def get_default_config() -> dict[str, Any]:
     return {
         "version": __version__,
@@ -130,20 +100,6 @@ def get_default_colors() -> dict[str, Any]:
     }
 
 
-def get_default_journal_path() -> str:
-    journal_data_path = xdg.BaseDirectory.save_data_path(XDG_RESOURCE) or home_dir()
-    return os.path.join(journal_data_path, DEFAULT_JOURNAL_NAME)
-
-
-def get_templates_path() -> Path:
-    # jrnl_xdg_resource_path is created by save_data_path if it does not exist
-    jrnl_xdg_resource_path = Path(xdg.BaseDirectory.save_data_path(XDG_RESOURCE))
-    jrnl_templates_path = jrnl_xdg_resource_path / "templates"
-    # Create the directory if needed.
-    jrnl_templates_path.mkdir(exist_ok=True)
-    return jrnl_templates_path
-
-
 def scope_config(config: dict, journal_name: str) -> dict:
     if journal_name not in config["journals"]:
         return config

jrnl/controller.py

@@ -11,10 +11,10 @@ from jrnl import time
 from jrnl.config import DEFAULT_JOURNAL_KEY
 from jrnl.config import get_config_path
 from jrnl.config import get_journal_name
-from jrnl.config import get_templates_path
 from jrnl.config import scope_config
 from jrnl.editor import get_text_from_editor
 from jrnl.editor import get_text_from_stdin
+from jrnl.editor import read_template_file
 from jrnl.exception import JrnlException
 from jrnl.journals import open_journal
 from jrnl.messages import Message
@@ -23,7 +23,6 @@ from jrnl.messages import MsgText
 from jrnl.output import print_msg
 from jrnl.output import print_msgs
 from jrnl.override import apply_overrides
-from jrnl.path import absolute_path
 
 if TYPE_CHECKING:
     from argparse import Namespace
@@ -130,74 +129,6 @@ def _is_append_mode(args: "Namespace", config: dict, **kwargs) -> bool:
     return append_mode
 
 
-def _read_template_file(template_arg: str, template_path_from_config: str) -> str:
-    """
-    This function is called when either a template file is passed with --template, or config.template is set.
-
-    The processing logic is:
-        If --template was not used: Load the global template file.
-        If --template was used:
-            * Check $XDG_DATA_HOME/jrnl/templates/template_arg.
-            * Check template_arg as an absolute / relative path.
-
-        If a file is found, its contents are returned as a string.
-        If not, a JrnlException is raised.
-    """
-    logging.debug(
-        "Append mode: Either a template arg was passed, or the global config is set."
-    )
-
-    # If filename is unset, we are in this flow due to a global template being configured
-    if not template_arg:
-        logging.debug("Append mode: Global template configuration detected.")
-        global_template_path = absolute_path(template_path_from_config)
-        try:
-            with open(global_template_path, encoding="utf-8") as f:
-                template_data = f.read()
-                return template_data
-        except FileNotFoundError:
-            raise JrnlException(
-                Message(
-                    MsgText.CantReadTemplateGlobalConfig,
-                    MsgStyle.ERROR,
-                    {
-                        "global_template_path": global_template_path,
-                    },
-                )
-            )
-    else:  # A template CLI arg was passed.
-        logging.debug("Trying to load template from $XDG_DATA_HOME/jrnl/templates/")
-        jrnl_template_dir = get_templates_path()
-        logging.debug(f"Append mode: jrnl templates directory: {jrnl_template_dir}")
-        template_path = jrnl_template_dir / template_arg
-        try:
-            with open(template_path, encoding="utf-8") as f:
-                template_data = f.read()
-                return template_data
-        except FileNotFoundError:
-            logging.debug(
-                f"Couldn't open {template_path}. Treating --template argument like a local / abs path."
-            )
-            pass
-
-        normalized_template_arg_filepath = absolute_path(template_arg)
-        try:
-            with open(normalized_template_arg_filepath, encoding="utf-8") as f:
-                template_data = f.read()
-                return template_data
-        except FileNotFoundError:
-            raise JrnlException(
-                Message(
-                    MsgText.CantReadTemplateCLIArg,
-                    MsgStyle.ERROR,
-                    {
-                        "normalized_template_arg_filepath": normalized_template_arg_filepath,
-                        "jrnl_template_dir": template_path,
-                    },
-                )
-            )
-
-
 def append_mode(args: "Namespace", config: dict, journal: "Journal", **kwargs) -> None:
     """
     Gets input from the user to write to the journal
@@ -210,26 +141,22 @@ def append_mode(args: "Namespace", config: dict, journal: "Journal", **kwargs) -
     """
     logging.debug("Append mode: starting")
 
-    if args.template or config["template"]:
+    template_text = _get_template(args, config)
-        logging.debug(f"Append mode: template CLI arg detected: {args.template}")
+
-        # Read template file and pass as raw text into the composer
+    if args.text:
-        template_data = _read_template_file(args.template, config["template"])
-        raw = _write_in_editor(config, template_data)
-        if raw == template_data:
-            logging.error("Append mode: raw text was the same as the template")
-            raise JrnlException(Message(MsgText.NoChangesToTemplate, MsgStyle.NORMAL))
-    elif args.text:
         logging.debug(f"Append mode: cli text detected: {args.text}")
         raw = " ".join(args.text).strip()
         if args.edit:
             raw = _write_in_editor(config, raw)
-
     elif not sys.stdin.isatty():
         logging.debug("Append mode: receiving piped text")
         raw = sys.stdin.read()
-
     else:
-        raw = _write_in_editor(config)
+        raw = _write_in_editor(config, template_text)
+
+    if template_text is not None and raw == template_text:
+        logging.error("Append mode: raw text was the same as the template")
+        raise JrnlException(Message(MsgText.NoChangesToTemplate, MsgStyle.NORMAL))
 
     if not raw or raw.isspace():
         logging.error("Append mode: couldn't get raw text or entry was empty")
@@ -251,6 +178,21 @@ def append_mode(args: "Namespace", config: dict, journal: "Journal", **kwargs) -
     logging.debug("Append mode: completed journal.write()")
 
 
+def _get_template(args, config) -> str:
+    # Read template file and pass as raw text into the composer
+    logging.debug(
+        f"Get template:\n--template: {args.template}\nfrom config: {config.get('template')}"
+    )
+    template_path = args.template or config.get("template")
+
+    template_text = None
+
+    if template_path:
+        template_text = read_template_file(template_path)
+
+    return template_text
+
+
 def search_mode(args: "Namespace", journal: "Journal", **kwargs) -> None:
     """
     Search for entries in a journal, and return the

jrnl/editor.py

@@ -15,6 +15,8 @@ from jrnl.messages import MsgText
 from jrnl.os_compat import on_windows
 from jrnl.os_compat import split_args
 from jrnl.output import print_msg
+from jrnl.path import absolute_path
+from jrnl.path import get_templates_path
 
 
 def get_text_from_editor(config: dict, template: str = "") -> str:
@@ -73,3 +75,46 @@ def get_text_from_stdin() -> str:
         )
 
     return raw
+
+
+def get_template_path(template_path: str, jrnl_template_dir: str) -> str:
+    actual_template_path = os.path.join(jrnl_template_dir, template_path)
+    if not os.path.exists(actual_template_path):
+        logging.debug(
+            f"Couldn't open {actual_template_path}. Treating template path like a local / abs path."
+        )
+        actual_template_path = absolute_path(template_path)
+
+    return actual_template_path
+
+
+def read_template_file(template_path: str) -> str:
+    """
+    Reads the template file given a template path in this order:
+
+        * Check $XDG_DATA_HOME/jrnl/templates/template_path.
+        * Check template_arg as an absolute / relative path.
+
+    If a file is found, its contents are returned as a string.
+    If not, a JrnlException is raised.
+    """
+
+    jrnl_template_dir = get_templates_path()
+    actual_template_path = get_template_path(template_path, jrnl_template_dir)
+
+    try:
+        with open(actual_template_path, encoding="utf-8") as f:
+            template_data = f.read()
+            return template_data
+    except FileNotFoundError:
+        raise JrnlException(
+            Message(
+                MsgText.CantReadTemplate,
+                MsgStyle.ERROR,
+                {
+                    "template_path": template_path,
+                    "actual_template_path": actual_template_path,
+                    "jrnl_template_dir": str(jrnl_template_dir) + os.sep,
+                },
+            )
+        )

jrnl/messages/MsgText.py

@@ -105,16 +105,12 @@ class MsgText(Enum):
 
     KeyboardInterruptMsg = "Aborted by user"
 
-    CantReadTemplateGlobalConfig = """
+    CantReadTemplate = """
-        Could not read template file defined in config:
+        Unable to find a template file {template_path}.
-        {global_template_path}
-        """
 
-    CantReadTemplateCLIArg = """
+        The following paths were checked:
-        Unable to find a template file based on the passed arg, and no global template was detected.
+         * {jrnl_template_dir}{template_path}
-        The following filepaths were checked:
+         * {actual_template_path}
-        jrnl XDG Template Directory : {jrnl_template_dir}
-        Local Filepath              : {normalized_template_arg_filepath}
         """
 
     NoNamedJournal = "No '{journal_name}' journal configured\n{journals}"

jrnl/output.py

@@ -39,7 +39,10 @@ def journal_list_to_yaml(journal_list: dict) -> str:
     from ruamel.yaml import YAML
 
     output = StringIO()
-    YAML().dump(journal_list, output)
+    dumper = YAML()
+    dumper.width = 1000
+    dumper.dump(journal_list, output)
+
     return output.getvalue()
 
 

jrnl/path.py

@@ -2,6 +2,19 @@
 # License: https://www.gnu.org/licenses/gpl-3.0.html
 
 import os.path
+from pathlib import Path
+
+import xdg.BaseDirectory
+
+from jrnl.exception import JrnlException
+from jrnl.messages import Message
+from jrnl.messages import MsgStyle
+from jrnl.messages import MsgText
+
+# Constants
+XDG_RESOURCE = "jrnl"
+DEFAULT_CONFIG_NAME = "jrnl.yaml"
+DEFAULT_JOURNAL_NAME = "journal.txt"
 
 
 def home_dir() -> str:
@@ -14,3 +27,46 @@ def expand_path(path: str) -> str:
 
 def absolute_path(path: str) -> str:
     return os.path.abspath(expand_path(path))
+
+
+def get_default_journal_path() -> str:
+    journal_data_path = xdg.BaseDirectory.save_data_path(XDG_RESOURCE) or home_dir()
+    return os.path.join(journal_data_path, DEFAULT_JOURNAL_NAME)
+
+
+def get_templates_path() -> str:
+    """
+    Get the path to the XDG templates directory. Creates the directory if it
+    doesn't exist.
+    """
+    # jrnl_xdg_resource_path is created by save_data_path if it does not exist
+    jrnl_xdg_resource_path = Path(xdg.BaseDirectory.save_data_path(XDG_RESOURCE))
+    jrnl_templates_path = jrnl_xdg_resource_path / "templates"
+    # Create the directory if needed.
+    jrnl_templates_path.mkdir(exist_ok=True)
+    return str(jrnl_templates_path)
+
+
+def get_config_directory() -> str:
+    try:
+        return xdg.BaseDirectory.save_config_path(XDG_RESOURCE)
+    except FileExistsError:
+        raise JrnlException(
+            Message(
+                MsgText.ConfigDirectoryIsFile,
+                MsgStyle.ERROR,
+                {
+                    "config_directory_path": os.path.join(
+                        xdg.BaseDirectory.xdg_config_home, XDG_RESOURCE
+                    )
+                },
+            ),
+        )
+
+
+def get_config_path() -> str:
+    try:
+        config_directory_path = get_config_directory()
+    except JrnlException:
+        return os.path.join(home_dir(), DEFAULT_CONFIG_NAME)
+    return os.path.join(config_directory_path, DEFAULT_CONFIG_NAME)

poetry.lock

@@ -1598,18 +1598,18 @@ jupyter = ["ipywidgets (>=7.5.1,<9)"]
 
 [[package]]
 name = "ruamel-yaml"
-version = "0.17.21"
+version = "0.17.24"
 description = "ruamel.yaml is a YAML parser/emitter that supports roundtrip preservation of comments, seq/map flow style, and map key order"
 category = "main"
 optional = false
 python-versions = ">=3"
 files = [
-    {file = "ruamel.yaml-0.17.21-py3-none-any.whl", hash = "sha256:742b35d3d665023981bd6d16b3d24248ce5df75fdb4e2924e93a05c1f8b61ca7"},
+    {file = "ruamel.yaml-0.17.24-py3-none-any.whl", hash = "sha256:f251bd9096207af604af69d6495c3c650a3338d0493d27b04bc55477d7a884ed"},
-    {file = "ruamel.yaml-0.17.21.tar.gz", hash = "sha256:8b7ce697a2f212752a35c1ac414471dc16c424c9573be4926b56ff3f5d23b7af"},
+    {file = "ruamel.yaml-0.17.24.tar.gz", hash = "sha256:90e398ee24524ebe20fc48cd1861cedd25520457b9a36cfb548613e57fde30a0"},
 ]
 
 [package.dependencies]
-"ruamel.yaml.clib" = {version = ">=0.2.6", markers = "platform_python_implementation == \"CPython\" and python_version < \"3.11\""}
+"ruamel.yaml.clib" = {version = ">=0.2.7", markers = "platform_python_implementation == \"CPython\" and python_version < \"3.12\""}
 
 [package.extras]
 docs = ["ryd"]
@@ -1953,4 +1953,4 @@ testing = ["flake8 (<5)", "func-timeout", "jaraco.functools", "jaraco.itertools"
 [metadata]
 lock-version = "2.0"
 python-versions = ">=3.10.0, <3.13"
-content-hash = "1306520c524e8cf20ca672e3b77eb0aa1fcc67e5994c86bd99a293415a0795b8"
+content-hash = "037329f79e6d33799cf05f4c71e15ea17368143eeb5e31adf59325c44e46fb45"

pyproject.toml

@@ -36,7 +36,7 @@ keyring = ">=21.0"       # https://github.com/jaraco/keyring#integration
 parsedatetime = ">=2.6"
 python-dateutil = "^2.8" # https://github.com/dateutil/dateutil/blob/master/RELEASING
 pyxdg = ">=0.27.0"
-"ruamel.yaml" = "0.17.21" # locked to this version until bug is resolved: https://github.com/jrnl-org/jrnl/issues/1736
+"ruamel.yaml" = ">=0.17.22"
 rich = ">=12.2.0, <14.0.0"
 
 # dayone-only deps

tests/bdd/features/template.feature

@@ -38,7 +38,7 @@ Feature: Using templates
         And we use the password "test" if prompted
         When we run "jrnl --template this_template_does_not_exist.template"
         Then we should get an error
-        Then the error output should contain "Unable to find a template file based on the passed arg"
+        Then the error output should contain "Unable to find a template file"
 
         Examples: configs
         | config_file          |

tests/lib/fixtures.py

@@ -183,10 +183,10 @@ def mock_default_journal_path(temp_dir):
 
 @fixture
 def mock_default_templates_path(temp_dir):
-    templates_path = Path(temp_dir.name, "templates")
+    templates_path = os.path.join(temp_dir.name, "templates")
     return {
         "get_templates_path": lambda: patch(
-            "jrnl.controller.get_templates_path", return_value=templates_path
+            "jrnl.editor.get_templates_path", return_value=templates_path
         ),
     }
 

tests/unit/test_editor.py

@@ -0,0 +1,45 @@
+# Copyright © 2012-2023 jrnl contributors
+# License: https://www.gnu.org/licenses/gpl-3.0.html
+
+import os
+from unittest.mock import mock_open
+from unittest.mock import patch
+
+import pytest
+
+from jrnl.editor import get_template_path
+from jrnl.editor import read_template_file
+from jrnl.exception import JrnlException
+
+
+@patch(
+    "os.getcwd", side_effect="/"
+)  # prevent failures in CI if current directory has been deleted
+@patch("builtins.open", side_effect=FileNotFoundError())
+def test_read_template_file_with_no_file_raises_exception(mock_open, mock_getcwd):
+    with pytest.raises(JrnlException) as ex:
+        read_template_file("invalid_file.txt")
+        assert isinstance(ex.value, JrnlException)
+
+
+@patch(
+    "os.getcwd", side_effect="/"
+)  # prevent failures in CI if current directory has been deleted
+@patch("builtins.open", new_callable=mock_open, read_data="template text")
+def test_read_template_file_with_valid_file_returns_text(mock_file, mock_getcwd):
+    assert read_template_file("valid_file.txt") == "template text"
+
+
+def test_get_template_path_when_exists_returns_correct_path():
+    with patch("os.path.exists", return_value=True):
+        output = get_template_path("template", "templatepath")
+
+    assert output == os.path.join("templatepath", "template")
+
+
+@patch("jrnl.editor.absolute_path")
+def test_get_template_path_when_doesnt_exist_returns_correct_path(mock_absolute_paths):
+    with patch("os.path.exists", return_value=False):
+        output = get_template_path("template", "templatepath")
+
+    assert output == mock_absolute_paths.return_value