From 4a7057c0381c59f73d15d14e17e5ad02cbf77ae3 Mon Sep 17 00:00:00 2001 From: Micah Jerome Ellison Date: Sat, 20 May 2023 16:13:24 -0700 Subject: [PATCH] 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 --- jrnl/config.py | 48 +------------ jrnl/controller.py | 106 +++++++--------------------- jrnl/editor.py | 45 ++++++++++++ jrnl/messages/MsgText.py | 14 ++-- jrnl/output.py | 5 +- jrnl/path.py | 56 +++++++++++++++ poetry.lock | 10 +-- pyproject.toml | 2 +- tests/bdd/features/template.feature | 2 +- tests/lib/fixtures.py | 4 +- tests/unit/test_editor.py | 45 ++++++++++++ 11 files changed, 190 insertions(+), 147 deletions(-) create mode 100644 tests/unit/test_editor.py diff --git a/jrnl/config.py b/jrnl/config.py index 6cb4bdc3..a2f0885c 100644 --- a/jrnl/config.py +++ b/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 diff --git a/jrnl/controller.py b/jrnl/controller.py index 07fdeab6..e0e222b5 100644 --- a/jrnl/controller.py +++ b/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"]: - logging.debug(f"Append mode: template CLI arg detected: {args.template}") - # Read template file and pass as raw text into the composer - 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: + template_text = _get_template(args, config) + + if 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 diff --git a/jrnl/editor.py b/jrnl/editor.py index adde528f..cbcf6207 100644 --- a/jrnl/editor.py +++ b/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, + }, + ) + ) diff --git a/jrnl/messages/MsgText.py b/jrnl/messages/MsgText.py index 1310231b..c8ac5b04 100644 --- a/jrnl/messages/MsgText.py +++ b/jrnl/messages/MsgText.py @@ -105,16 +105,12 @@ class MsgText(Enum): KeyboardInterruptMsg = "Aborted by user" - CantReadTemplateGlobalConfig = """ - Could not read template file defined in config: - {global_template_path} - """ + CantReadTemplate = """ + Unable to find a template file {template_path}. - CantReadTemplateCLIArg = """ - Unable to find a template file based on the passed arg, and no global template was detected. - The following filepaths were checked: - jrnl XDG Template Directory : {jrnl_template_dir} - Local Filepath : {normalized_template_arg_filepath} + The following paths were checked: + * {jrnl_template_dir}{template_path} + * {actual_template_path} """ NoNamedJournal = "No '{journal_name}' journal configured\n{journals}" diff --git a/jrnl/output.py b/jrnl/output.py index f11f2382..2d7064cb 100644 --- a/jrnl/output.py +++ b/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() diff --git a/jrnl/path.py b/jrnl/path.py index 306e34cc..4361cf97 100644 --- a/jrnl/path.py +++ b/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) diff --git a/poetry.lock b/poetry.lock index b6d7d24d..952a1849 100644 --- a/poetry.lock +++ b/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.21.tar.gz", hash = "sha256:8b7ce697a2f212752a35c1ac414471dc16c424c9573be4926b56ff3f5d23b7af"}, + {file = "ruamel.yaml-0.17.24-py3-none-any.whl", hash = "sha256:f251bd9096207af604af69d6495c3c650a3338d0493d27b04bc55477d7a884ed"}, + {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" diff --git a/pyproject.toml b/pyproject.toml index 675d4f40..0938c5a8 100644 --- a/pyproject.toml +++ b/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 diff --git a/tests/bdd/features/template.feature b/tests/bdd/features/template.feature index a6a69b9f..ab13115a 100644 --- a/tests/bdd/features/template.feature +++ b/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 | diff --git a/tests/lib/fixtures.py b/tests/lib/fixtures.py index b9cf0ea4..168a8007 100644 --- a/tests/lib/fixtures.py +++ b/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 ), } diff --git a/tests/unit/test_editor.py b/tests/unit/test_editor.py new file mode 100644 index 00000000..5c21daca --- /dev/null +++ b/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