Allow custom file extensions for directory-based journal #1789
Conversation
There was a problem hiding this comment.
Thanks for the PR. I added some initial comments below that I hope should help. Let me know if you have any questions about any of it.
We'll also need new tests to accept this PR. I recommend taking a look at /jrnl/tests/bdd/features/file_storage.feature and testing for a new extension. It will probably need a new sample config and sample folder journal data as well. I did the same thing in a PR #1697 a few months ago, which I think should help show what's needed, but again, if you have any questions, feel free to respond here and I'll be happy to help.
| @@ -45,6 +45,8 @@ def open(self) -> "Folder": | |||
|
|
|||
| def write(self) -> None: | |||
| """Writes only the entries that have been modified into proper files.""" | |||
| if self.config["extension"] is not None: | |||
There was a problem hiding this comment.
This line is one of the reasons the tests are failing -- the "extension" key doesn't always exist in every config, so there's a KeyError when trying to retrieve it. Checking self.config.get("extension") instead of self.config["extension"] should do the trick.
| @@ -45,6 +45,8 @@ def open(self) -> "Folder": | |||
|
|
|||
| def write(self) -> None: | |||
| """Writes only the entries that have been modified into proper files.""" | |||
| if self.config["extension"] is not None: | |||
| EXTENSION = self.config["extension"] | |||
There was a problem hiding this comment.
A little style note - could you keep this extension variable lower case? Upper case is meant for constants.
| @@ -121,12 +123,14 @@ def parse_editable_str(self, edited: str) -> None: | |||
| self.entries = mod_entries | |||
|
|
|||
| @staticmethod | |||
| def _get_files(journal_path: str) -> list[str]: | |||
| def _get_files(self, journal_path: str) -> list[str]: | |||
There was a problem hiding this comment.
Since this is a static method, it shouldn't take self. Instead, I think it would be better to pass the extension as a argument with a default value of None, so the _get_files method doesn't have to be "aware" of how the config works.
| def _get_day_files(self, extension, path: pathlib.Path) -> list[str]: | ||
| EXTENSION = extension | ||
| DAY_PATTERN = "[0-3][0-9]." + EXTENSION | ||
| for child in path.iterdir(): |
There was a problem hiding this comment.
I would prefer to keep the glob method rather than using iterdir and regex, for performance reasons. We've seen issues in the past where unpredictable things can happen for users when pointing jrnl to a large directory, and iterdir has been the culprit since it iterates through all files instead of just the expected files.
We can still use the glob approach here by adding the configured extension the day pattern at the start of this method, though.
This PR seeks to resolve #1289, which I opened a while back. This PR utilizes a new config option
extensionwhich permits the user to change the file extensionNow, when a directory-based journal is searched or a new entry is created, jrnl will recognize files with the custom file extension or create a file using the custom file extension, respectively.This PR makes use of regex for the filename recognition.
A previous version of this code attempted to implement the possibility to use an array of extensions, but eventually I bumped into the rigid structure alluded to in #1289 (comment). I reviewed the code in order to understand what was happening. Now that I understand what the code is doing, I see that implementing an array would require a much larger amount of work. A single file extension change should be much more manageable at this time.1
I have tested basic functionality and the code is working, but this PR is flagged as a draft because I have to learn how to handle output from
poe testand then review that output. I am opening this PR as draft in order to share my work and to receive help.When setting "extension: txt", I get:
Several of these appear to be related to me having to pass along additional positional arguments. Hopefully I can resolve these with time.
Checklist
for the same issue.
Footnotes
For posterity's sake, when implementing an array of extensions, I found that the code worked—except and unless there are multiple files on the same day with different extensions. The code is looking for a single file extension in part because once the entries are read from the files, the file paths are no longer passed along to the rest of the code, rather the file paths are retrieved and then reassigned. And so when attempting to use such functionality, the
--deleteoption more or less no longer functions properly—not on .txt files or the first extension set in the config file—but on additional extensions beyond these. Without being able to specify the file, the delete option ceases working as intended. Perhaps to summarize, jrnl's internal code operates off of journal entries, not off of files. ↩