Add ability to use template with `--template` #1667

alichtman · 2023-01-23T11:09:09Z

You can use this CLI argument in two ways:

# Pass a filepath directly (relative or absolute)
$ jrnl --template local_file.md
$ jrnl --template /tmp/abs_path_file.md

# Or use the default XDG template directory
$ mkdir -p $XDG_DATA_HOME/jrnl/templates
$ echo "Hi! I'm a template!" > $XDG_DATA_HOME/jrnl/templates/test.md
$ jrnl --template test.md

I've updated the docs for the Templates feature.

Also, see https://github.com/alichtman/jrnl-templates

Checklist

I have read the contributing doc.
I have included a link to the relevant issue number.
I have checked to ensure there aren't other open pull requests
for the same issue.
I have written new tests for these changes, as needed.

jrnl/args.py

alichtman · 2023-01-23T12:30:11Z

tests/unit/test_parse_args.py

@@ -39,6 +39,7 @@ def expected_args(**kwargs):
        "start_date": None,
        "strict": False,
        "tags": False,
+        "template": None,


It's odd that the tests fail when this value is set to False. I didn't expect that.

micahellison

Thanks for this PR. I like the --template idea and I think it's a lot more fluent and easy to use than the current method of --config-overide template file. I also really appreciate the changes to the docs here too.

These are a few different changes, though, and I feel particularly hesitant about the environment variable change. Could you remove that part from this PR? I think we need a broader conversation about how jrnl could handle environment variables, and I think that will take some time, but I don't want it to hold up this nice --template change.

I think we could use some BDD tests for --template as well. There's a new template.feature file in there that could use these tests. Let me know if you'd like any help with that.

jrnl/controller.py

alichtman · 2023-03-13T08:20:23Z

I've made the default template path $XDG_DATA_HOME/jrnl/templates and updated the docs. Everything works based on my manual testing, and I've added some bdd tests.

I'm unsure how to handle two test cases:

Testing that involves $XDG_DATA_HOME/jrnl/templates without actually messing with the system's $XDG_DATA_HOME/jrnl/templates folder.
Testing that involves an arbitrary absolute filepath.

Do you have any ideas about how I can mock that behavior out?

And it's unclear to me why test_template_local_filepath_should_be_used_in_new_entry is failing. Will debug more later.

Update jrnl/args.py

tests/bdd/features/template.feature

micahellison · 2023-03-13T16:00:21Z

This looks nice! Thanks for your work on this. I'll plan on taking a deeper look at this later but wanted to get to your questions for now.

Testing that involves $XDG_DATA_HOME/jrnl/templates without actually messing with the system's $XDG_DATA_HOME/jrnl/templates folder.

My instinct would be to create a new fixture in fixtures.py patching your new get_templates_path method to return a subdirectory of the temp_dir fixture, like the mock_default_journal_path fixture, then add it to the cli_run fixture so the patch is applied in the "we run" step. After that, a new given step could populate that directory with a template from the test data folder. Like, "Given we copy the {template_file_name} template to the $XDG_DATA_HOME/jrnl/templates folder".

Testing that involves an arbitrary absolute filepath.

I'm not sure. I'm looking through other tests we've done with path inputs, such as --config-file and the installation wizard, and I don't see anything using absolute paths. One might argue that absolute vs. relative paths are a concern of the Python libraries rather than jrnl itself and therefore don't need to be tested by us, though it still could be nice if we had an approach for this since path manipulation bugs are common, especially in cross-platform apps. Maybe @wren has some thoughts on this. Either way, I'd be fine accepting this PR without absolute file path tests.

And it's unclear to me why test_template_local_filepath_should_be_used_in_new_entry is failing.

Due to #1653, jrnl now ignores entries that are exactly the same as the template, so the entry isn't being added. You can copy a couple lines from an earlier test in template.feature to add some text to the entry and get it working:

        And we append to the editor if opened
            This is an addition to a templated entry

alichtman · 2023-03-14T02:28:23Z

Due to #1653, jrnl now ignores entries that are exactly the same as the template, so the entry isn't being added. You can copy a couple lines from an earlier test in template.feature to add some text to the entry and get it working:

Fixed.

My instinct would be to create a new fixture in fixtures.py patching your new get_templates_path method to return a subdirectory of the temp_dir fixture, like the mock_default_journal_path fixture, then add it to the cli_run fixture so the patch is applied in the "we run" step. After that, a new given step could populate that directory with a template from the test data folder. Like, "Given we copy the {template_file_name} template to the $XDG_DATA_HOME/jrnl/templates folder".

Makes sense to me. What I've got in the last commit seems like it should work, but the get_templates_path function isn't being properly mocked (and is instead returning my real XDG-based templates path). Any ideas on what I missed?

One might argue that absolute vs. relative paths are a concern of the Python libraries rather than jrnl itself and therefore don't need to be tested by us

I think Pathlib does all the work here, and it's acceptable to drop this test entirely. Plus, the risk is low: worst case, a jrnl entry won't be prepopulated with a template.

tests/lib/fixtures.py

micahellison

Just one note about a long function. Otherwise, it looks good. Thanks for your work on this!

micahellison · 2023-03-18T21:54:44Z

jrnl/controller.py

@@ -123,9 +124,78 @@ def _is_write_mode(args: "Namespace", config: dict, **kwargs) -> bool:
    return write_mode


+def _read_template_file(template_arg: str, template_path_from_config: str) -> str:


I'm thinking that this function should be split up into shorter functions and at least some of them should be moved elsewhere. editor.py has previously been the only file involved with templates and I think some of these concerns belong there. Ideally, the controller is just telling other classes what to do, rather than doing the work itself.

Though let me know if you're not interested in that work -- we're happy to do it as well.

I'm thinking that this function should be split up into shorter functions and at least some of them should be moved elsewhere.

Seems reasonable enough to me. I don't have a strong opinion on it.

Though let me know if you're not interested in that work -- we're happy to do it as well.

I'll pass on implementing it myself :) I'm already running a serious open-source time deficit.

alichtman

I'd be alright shipping this as is, and cutting a follow-up task for the changes you suggested, @micahellison.

Thanks for the reviews!

alichtman · 2023-03-18T22:21:06Z

jrnl/controller.py

@@ -123,9 +124,78 @@ def _is_write_mode(args: "Namespace", config: dict, **kwargs) -> bool:
    return write_mode


+def _read_template_file(template_arg: str, template_path_from_config: str) -> str:


I'm thinking that this function should be split up into shorter functions and at least some of them should be moved elsewhere.

Seems reasonable enough to me. I don't have a strong opinion on it.

Though let me know if you're not interested in that work -- we're happy to do it as well.

I'll pass on implementing it myself :) I'm already running a serious open-source time deficit.

micahellison

Thanks for all your work on this!

alichtman force-pushed the develop branch from e561dfd to 20b7172 Compare January 23, 2023 12:06

alichtman marked this pull request as ready for review January 23, 2023 12:13

alichtman force-pushed the develop branch from 20b7172 to c104ae7 Compare January 23, 2023 12:17

alichtman commented Jan 23, 2023

View reviewed changes

jrnl/args.py Outdated Show resolved Hide resolved

alichtman force-pushed the develop branch 2 times, most recently from a881181 to 8c4550d Compare January 23, 2023 12:28

alichtman commented Jan 23, 2023

View reviewed changes

alichtman force-pushed the develop branch 2 times, most recently from f5ae7b7 to 813b4e7 Compare January 23, 2023 12:35

alichtman changed the title ~~Add ability to pass template path with --template~~ Add ability to pass template path with --template Jan 23, 2023

alichtman changed the title ~~Add ability to pass template path with --template~~ Add ability to pass template path with --template and $JRNL_TEMPLATE_DIR Jan 23, 2023

micahellison requested changes Jan 28, 2023

View reviewed changes

jrnl/controller.py Show resolved Hide resolved

alichtman force-pushed the develop branch 2 times, most recently from 06a785a to 3f2090e Compare March 13, 2023 07:27

alichtman changed the title ~~Add ability to pass template path with --template and $JRNL_TEMPLATE_DIR~~ Add ability to pass template path with --template Mar 13, 2023

alichtman force-pushed the develop branch 2 times, most recently from c08301a to 0b8380d Compare March 13, 2023 08:19

Add ability to pass template path with --template

1d9ca76

Update jrnl/args.py

alichtman force-pushed the develop branch from 0b8380d to 1d9ca76 Compare March 13, 2023 08:23

alichtman requested a review from micahellison March 13, 2023 08:24

alichtman force-pushed the develop branch from cb182af to ebbdb55 Compare March 13, 2023 08:27

Merge branch 'develop' into develop

dbc9874

alichtman force-pushed the develop branch from ebbdb55 to dbc9874 Compare March 13, 2023 08:37

alichtman commented Mar 13, 2023

View reviewed changes

tests/bdd/features/template.feature Show resolved Hide resolved

alichtman changed the title ~~Add ability to pass template path with --template~~ Add ability to use template with --template Mar 13, 2023

alichtman commented Mar 14, 2023

View reviewed changes

tests/lib/fixtures.py Outdated Show resolved Hide resolved

Fix tests

5908ae6

alichtman force-pushed the develop branch from 3714c99 to 5908ae6 Compare March 16, 2023 01:44

micahellison requested changes Mar 18, 2023

View reviewed changes

alichtman commented Mar 18, 2023

View reviewed changes

alichtman requested a review from micahellison March 22, 2023 23:13

micahellison approved these changes Mar 25, 2023

View reviewed changes

wren added the enhancement New feature or request label Mar 25, 2023

micahellison merged commit a2b217f into jrnl-org:develop Mar 25, 2023

micahellison mentioned this pull request Mar 25, 2023

Refactor --template code #1711

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to use template with `--template` #1667

Add ability to use template with `--template` #1667

alichtman commented Jan 23, 2023 •

edited by micahellison

Loading

alichtman Jan 23, 2023 •

edited

Loading

micahellison left a comment

alichtman commented Mar 13, 2023 •

edited

Loading

micahellison commented Mar 13, 2023

alichtman commented Mar 14, 2023 •

edited

Loading

micahellison left a comment

micahellison Mar 18, 2023

alichtman Mar 18, 2023

alichtman left a comment

alichtman Mar 18, 2023

micahellison left a comment

		@@ -123,9 +124,78 @@ def _is_write_mode(args: "Namespace", config: dict, **kwargs) -> bool:
		return write_mode


		def _read_template_file(template_arg: str, template_path_from_config: str) -> str:

Add ability to use template with --template #1667

Add ability to use template with --template #1667

Conversation

alichtman commented Jan 23, 2023 • edited by micahellison Loading

Checklist

alichtman Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

micahellison left a comment

Choose a reason for hiding this comment

alichtman commented Mar 13, 2023 • edited Loading

micahellison commented Mar 13, 2023

alichtman commented Mar 14, 2023 • edited Loading

micahellison left a comment

Choose a reason for hiding this comment

micahellison Mar 18, 2023

Choose a reason for hiding this comment

alichtman Mar 18, 2023

Choose a reason for hiding this comment

alichtman left a comment

Choose a reason for hiding this comment

alichtman Mar 18, 2023

Choose a reason for hiding this comment

micahellison left a comment

Choose a reason for hiding this comment

Add ability to use template with `--template` #1667

Add ability to use template with `--template` #1667

alichtman commented Jan 23, 2023 •

edited by micahellison

Loading

alichtman Jan 23, 2023 •

edited

Loading

alichtman commented Mar 13, 2023 •

edited

Loading

alichtman commented Mar 14, 2023 •

edited

Loading