Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jrnl/Journal.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from . import Entry
from . import time
from .prompt import yesno
from .path import expand_path


class Tag:
Expand Down Expand Up @@ -404,7 +405,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"]:
Expand Down
9 changes: 3 additions & 6 deletions jrnl/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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"
Expand Down Expand Up @@ -83,9 +84,7 @@ def get_config_path():
),
)

return os.path.join(
config_directory_path or os.path.expanduser("~"), DEFAULT_CONFIG_NAME
)
return os.path.join(config_directory_path or home_dir(), DEFAULT_CONFIG_NAME)


def get_default_config():
Expand All @@ -112,9 +111,7 @@ def get_default_config():


def get_default_journal_path():
journal_data_path = xdg.BaseDirectory.save_data_path(
XDG_RESOURCE
) or os.path.expanduser("~")
journal_data_path = xdg.BaseDirectory.save_data_path(XDG_RESOURCE) or home_dir()
return os.path.join(journal_data_path, DEFAULT_JOURNAL_NAME)


Expand Down
13 changes: 7 additions & 6 deletions jrnl/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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(
os.path.expandvars(journal_path)
)
default_config["journals"][DEFAULT_JOURNAL_KEY] = journal_path

# If the folder doesn't exist, create it
path = os.path.split(default_config["journals"][DEFAULT_JOURNAL_KEY])[0]
Expand Down Expand Up @@ -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]
7 changes: 5 additions & 2 deletions jrnl/jrnl.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
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
Expand Down Expand Up @@ -195,16 +196,18 @@ 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")
raise JrnlException(
Message(
MsgText.CantReadTemplate,
MsgType.ERROR,
{"template": config["template"]},
{"template": template_path},
)
)

Expand Down
13 changes: 13 additions & 0 deletions jrnl/path.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import os.path


def home_dir():
return os.path.expanduser("~")


def expand_path(path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do with just home_dir and expand_path as long as expand_path also runs abspath. Would you mind merging expand_path and absolute_path into one function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that at first because I assumed that it wouldn't make a difference but I think that's not a great idea for a couple reasons.

  1. It breaks the tests as currently written. A few BDD tests check the console output of the application and the output that gets parsed contains relative paths. That means that those tests would have to changed to parse the absolute path which is a bit more complicated.
  2. The absolute path is only needed once so it would add extra work to all the other calls.
  3. Console error messages are more readable when they display the relative path instead of the absolute path.

return os.path.expanduser(os.path.expandvars(path))


def absolute_path(path):
return os.path.abspath(expand_path(path))
11 changes: 5 additions & 6 deletions jrnl/upgrade.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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

Expand All @@ -22,7 +23,7 @@

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:
Expand Down Expand Up @@ -72,15 +73,13 @@ def upgrade_jrnl(config_path):

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)):
path = os.path.expanduser(path)
else:
if not os.path.exists(path):
print(f"\nError: {path} does not exist.")
continue

Expand Down
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ required_plugins = [
]
markers = [
"todo",
"skip_win",
"skip_posix",
"on_win",
"on_posix",
]
addopts = [
"--pdbcls=IPython.terminal.debugger:Pdb"
Expand Down
30 changes: 30 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand All @@ -15,11 +17,39 @@


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")
103 changes: 103 additions & 0 deletions tests/unit/test_path.py
Original file line number Diff line number Diff line change
@@ -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)