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

Allow custom file extensions for directory journal #1873

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

comkieffer
Copy link

This PR builds on #1789 (Closes issue #1289).

In short, it adds a new configuration option extension that controls the extension for the files created with a folder journal.

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.

@comkieffer comkieffer force-pushed the allow-custom-file-extensions-for-directory-journal branch 3 times, most recently from 5b5a3c4 to 386c222 Compare March 23, 2024 19:43
@thw26
Copy link

thw26 commented Apr 3, 2024

Thanks for building on my PR! I have not had the time to return to it since the review.

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to get to this PR, but also, thank you for the excellent PR. I really appreciate that you're modifying documentation, adding tests, and adding thoughtful changes within the norms of the existing code.

The GitHub Actions tests were broken when your PR was first submitted, but they should be workable now if you merge in develop. Otherwise, I added notes about a couple small tasks that should get these new tests passing.

tests/bdd/features/file_storage.feature Outdated Show resolved Hide resolved
Scenario: Adding entries to a Folder journal with a custom extension should generate date files
Given we use the config "empty_folder_with_extension.yaml"
When we run "jrnl 23 July 2013: Testing folder journal."
Then we should get no error
Copy link
Member

Choose a reason for hiding this comment

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

Congratulations, your test uncovered an existing bug! 🎉 I filed it in #1894.

Looks like empty_folder.yaml got past this issue by having an "empty" file in it: tests/data/journals/empty_folder/empty

Feel free to copy that "empty" file to tests/data/journals/empty_folder_with_extension/empty to get this working.

Copy link
Author

Choose a reason for hiding this comment

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

I can't replicate this. I'm not sure what might have caused the issue.

I re-ran the tests without the empty file in test/data/journals/empty_folder and the tests still passed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like bug #1894 is Windows-only, and this is combined with a limitation of git: since we can only commit files (not directories), that empty_folder doesn't exist on the remote repository until we push a file in it.

The test previously only tested that one of the expected files was
created.
@comkieffer comkieffer force-pushed the allow-custom-file-extensions-for-directory-journal branch from 386c222 to 8d9c27f Compare May 25, 2024 17:20
micahellison and others added 2 commits June 3, 2024 19:18
This also handles additional input for extensions in case the user
isn't fully aware of how extensions work
@wren wren force-pushed the allow-custom-file-extensions-for-directory-journal branch from 7c8a166 to cbf4ed5 Compare August 24, 2024 21:07
def _get_day_files(path: pathlib.Path) -> list[str]:
for child in path.glob(DAY_PATTERN):
def _get_day_files(path: pathlib.Path, extension: str) -> list[str]:
for child in path.glob(DAY_PATTERN + extension):
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 the only thing left on this PR is supporting a lack of dots in the extension. For instance, I'd expect this to work the same whether my config value is extension: md or extension: .md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants