Implement --change-time flag #1452
Conversation
|
@richardjs Still reviewing this, but I wanted to say that I really appreciate how well you matched our style, both in the code and in the tests. This is a very high quality PR. Thank you! |
jrnl/jrnl.py
Outdated
| @@ -169,6 +172,16 @@ def search_mode(args, journal, **kwargs): | |||
| # Where do the search results go? | |||
| if args.edit: | |||
| _edit_search_results(**kwargs) | |||
| # If we want to both edit and change time in one action | |||
| if args.change_time: | |||
| # Re-filter the journal enties (_edit_search_results puts | |||
There was a problem hiding this comment.
This is a good observation. We should (in some other PR, not this one) separate the filtering and recombining of entries so that each of these commands doesn't have to worry about it. Thanks for pointing this out.
| @@ -169,6 +172,16 @@ def search_mode(args, journal, **kwargs): | |||
| # Where do the search results go? | |||
| if args.edit: | |||
| _edit_search_results(**kwargs) | |||
| # If we want to both edit and change time in one action | |||
| if args.change_time: | |||
There was a problem hiding this comment.
This is a good addition but I have one slight change to request. If a user runs --edit and --change-time together, then --change-time should happen first, and the updated timestamps should be loaded into the editor.
This avoids the awkward use case of jrnl overwriting a user's manual changes from the editor after they save the file.
There was a problem hiding this comment.
@wren I actually had it that way first, but changed it because after we change the entries' dates, they may not be selected again by _search_journal. For example, if the user ran jrnl -on today --change-time tomorrow --edit, today's entries would be changed to tomorrow, and then rerunning _search_journal before the call to _edit_search_results would select nothing.
However, while writing this I realize that we can do something to save which entries were selected and pass them on to edit, maybe apart from _search_journal. I'll see what I can do.
There was a problem hiding this comment.
@wren , the change has been implemented!
| @@ -320,6 +338,18 @@ def _delete_search_results(journal, old_entries, **kwargs): | |||
| journal.write() | |||
|
|
|||
|
|
|||
| def _change_time_search_results(args, journal, old_entries, **kwargs): | |||
| # separate entries we are not editing | |||
There was a problem hiding this comment.
Can we add something similar here to the start of _delete_search_results? That way, if there are no search results, the user will get a message about how nothing is happening. Right now, this doesn't do anything, and is quiet about it, which can be confusing.
There was a problem hiding this comment.
I wondered about that; I wasn't sure if --delete was only warned because it was destructive (for example, --edit doesn't warn if nothing is selected), and I reasoned it wasn't exactly an error situation (we're technically performing successfully what was requested).
But I agree it has the potential to be confusing. I'll add it in!
There was a problem hiding this comment.
I added the message. I used the language "No entries to modify" because it sounded cleaner than "No entries to change time for" or similar, and it has the potential to be used in other situations. But I'm happy to use whatever you suggest. (Edit: Oh, and I also used MsgType.WARNING, but I'm happy to use ERROR like --delete.)
Unfortunately, I realized that if you run jrnl --change-time without anything else, it changes the time of every entry. So I'll fix that. In the meantime, I added a test for it (that currently fails).
There was a problem hiding this comment.
After considering this bug, I posted on #1429 with some considerations about how --change-time might benefit from some safeguards.
There was a problem hiding this comment.
Per @micahellison in #1429, I added a confirmation dialog to --change-time. There is one exception: if --edit and --change-time are used together with only one entry, no confirmation is requested. To my mind, this makes the amend-style action of --change-time smooth, while retaining the safety for larger operations.
But I can also see a clear case being made for confirmation even in that case (consistency, confirming what is still a destructive action, albeit on one entry, etc.), so I'm happy to remove that exception. I'm also wondering about adding some sort of output, e.g. "Changing time from X to Y", so the user can at least manually undo an error.
…arning and doesn't change anything
There was a problem hiding this comment.
Looks great! Just one small bug with the prompt while using --edit to fix.
Also, if you can add a test or two for using --change-time and --edit it would be great (a test like this would have caught the bug mentioned above).
That should be it; everything else looks great! Thank you!
| # modified by _change_time_search_results | ||
| selected_entries = [e for e in journal.entries] | ||
|
|
||
| no_change_time_prompt = len(journal.entries) == 1 |
There was a problem hiding this comment.
This doesn't work as intended right now. I think this line is supposed to reference selected_entries instead of journal.entries.
There was a problem hiding this comment.
@wren, can you clarify the bug you're seeing? selected_entries here is built from journal.entries, so they have the same length. selected_entries only exists so we can reset journal.entries to the original set of entries on line 188.
(I agree that a test should cover the functionality--just added one.)
There was a problem hiding this comment.
Hi, @richardjs. Back from vacation now; thanks for your patience!
Nevermind about this bug! I misinterpreted the requirement about when to prompt the user. I thought we were always skipping when there was only one entry selected, but I see now that it's only when the edit flag is also used. So, this works as expected. Thank you!
This implements #1429, for the reasons and in the manner described there.
It wasn't discussed in the issue, but I also handled the case where both
--editand--change-timeare used. I thought it was a useful enough feature that it was worth implementing (it's equivalent to the originally-described--amendflag in the issue). Unfortunately, the resulting code feels a little ad-hoc; I'm happy to improve it if you have suggestions. I'm also happy to drop that sub-feature if you don't think it's worth the special case.Checklist
for the same issue.