-
-
Notifications
You must be signed in to change notification settings - Fork 524
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
base: develop
Are you sure you want to change the base?
Allow custom file extensions for directory journal #1873
Conversation
5b5a3c4
to
386c222
Compare
Thanks for building on my PR! I have not had the time to return to it since the review. |
There was a problem hiding this 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.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
386c222
to
8d9c27f
Compare
This also handles additional input for extensions in case the user isn't fully aware of how extensions work
7c8a166
to
cbf4ed5
Compare
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): |
There was a problem hiding this comment.
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
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
for the same issue.