Add --delete for interactive removal of entries

[alichtman]

• Add switch for deleting entries. Works with filters, etc.
• Fix some minor style issues

Fix #434

# First, create a journal named "test"
$ python3 -m jrnl test First entry
$ python3 -m jrnl test Second entry
$ python3 -m jrnl test Third entry
$ python3 -m jrnl test --delete

Checklist

• The code change is tested and works locally.
• Tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?

[alichtman]

Turns out the python library I used for prompting doesn't support windows: magmax/python-inquirer#63

I could load an alternative library in that case, disable the feature, or find a different library that works on all platforms.

Edit: Pivot to this library: https://github.com/CITGuru/PyInquirer

[alichtman]

    # The input for this test is <SPACE><ENTER>y
    Scenario: --delete flag allows deletion of single entry
        Given we use the config "deletion.yaml"
        When we run "jrnl --delete"
        And we type " "
        And we type
            """

            y
            """
        When we run "jrnl -on 2019-10-29 -s"
        Then the output should not contain "2019-10-29 11:11 First entry."

This structure for the test doesn't work... Testing this feature as it is may not be possible.

[pspeter]

PyInquirer seems like a heavy dependency for a relatively simple feature like this.
Also, your PR adds another place which parses datetimes without using config["timeformat"], which we should try to avoid as time.parse() doesn't work with all formats.

Why don't we just do it like rm -i and keep it simple?
Something like:

def prompt_delete_entries(entries: List[Entry]):
  print("Confirm each entry you want to delete [N/y]:")
  to_delete: List[Entry] = []
  for entry in entries:
    response = input("jrnl: Delete entry '{}'? ".format(entry.pprint(short=True)))
    if response == "y":
      to_delete.append(entry)

  return to_delete

And then:

jrnl.entries = [e for e in jrnl.entries if e not in to_delete]
jrnl.write()

That would also be much easier to test.

[alichtman]

I believe it's a much better user experience with the interactive selection provided by PyInquirer. If I want to quickly scan a list of potentially hundreds of journal entries and select only one or two to delete, I'd much rather have the selection checkbox than have to [y/n] every single entry up until the one I'd like to remove (and then keymash n until I hit the end of the list...)

[pspeter]

jrnl already provides several options to limit your entries: -n, -from, -until, -starred, and tags. These seem sufficient to me to be honest.

Adding a complete command-line UI just for --delete creates an inconsistent user experience in my opinion.

[alichtman]

Adding a complete command-line UI just for --delete creates an inconsistent user experience in my opinion.

I see a natural progression to using this with --edit, as well. Any features that involve selecting some subset of journal entries before performing some other action with them (aside from maybe viewing) should have a nice selection UI.

I just don't think this looks good, and it's not easy to manage lots of entries with it. Additionally, you can't backstep if you make a mistake. You have to exit and restart the selection process.

[alichtman]

your PR adds another place which parses datetimes without using config["timeformat"], which we should try to avoid as time.parse() doesn't work with all formats.

This is a fair point though. I have to take a closer look at how to handle this.

[pspeter]

I may be wrong, but I think the main use case for this would be to delete the last entry. This would be very easy with my proposal, especially when adding a -y|--yes option to skip the questions:
jrnl -1 --delete -y

It might even make sense to make that the default when none of the selection options are specified (instead of putting up the whole journal for deletion).

Even if you are looking for a specific entry, then you probably saw that entry earlier and can look for it using its date:
jrnl -on <data> --delete

And then select the one from that day (probably selecting one of 1-5 entries).

If you are looking for a specific text to delete in the journal then the feature proposed in #463 would help. But if you need to really scan through the whole thing then maybe just going with jrnl --edit and dealing with it in your editor would be the better choice anyways.

Sorry if I'm being a downer here, I'm just personally not a big fan of 'GUIfying' a CLI ;)

[alichtman]

Even if you are looking for a specific entry, then you probably saw that entry earlier and can look for it using its date

I'd rather combine these steps into one command. Instead of going fishing for an entry, copying the date or a tag or something, and then running jrnl <search> --delete, I'd like to start fishing from jrnl --delete.

But if you need to really scan through the whole thing then maybe just going with jrnl --edit and dealing with it in your editor would be the better choice anyways.

I'm always scared of deleting partial entries / more than I want to when I pull up the raw journal to edit in vim. I always have to scan what I'm deleting multiple times just to be sure...

One nice aspect of the interactive selection option is that it's not possible to delete partial entries / corrupt the journal. Your solution works for small lists of entries, but I'd prefer mine for larger lists.

I do understand your concerns, however. Still thinking about how to best handle this. It'd be great to get some input from other contributors and users.

[wren]

This is a tough one. I think the introduction of an interactive UI is a bit too much for now, but you raise some very good points about how cumbersome it can be to deal with querying large amounts of journal entries for the right ones to delete. I need to think about this one a bit more.

@micahellison What do you think?

[micahellison]

I'm of a couple minds about this also. I was about to post that we should use filtering rather than an interactive tool, but then it hit me that one reason to delete an entry could be that a duplicate was made on the same specified time. No amount of filtering could rule out that duplicate, right? Having an entry-by-entry confirmation could deal with that situation, but it could also be really annoying for when a user is really sure that they want to delete a range of entries.

And yet, maybe this is what editing is for. After all, people have already been using this tool for seven years, and the edit functionality has been the way to do this. Maybe --delete could be for the simple use cases (such as the most recent entry, as @pspeter mentioned), and we recommend users use edit for more complicated situations.

As an aside, I think there is a place for interactive UIs that interact with jrnl and/or jrnl's data. I would love for this tool to be easily available for people that aren't comfortable with the command line. But I think we should take great care in figuring out their points of contact with jrnl (e.g. a plugin system), and setting them up in a way that minimizes their impact on jrnl's core functionality. So even with tricky use cases like the above, my instinct is to lean away from interactivity.

[alichtman]

But I think we should take great care in figuring out their points of contact with jrnl (e.g. a plugin system), and setting them up in a way that minimizes their impact on jrnl's core functionality.

I would agree that having a plugin system is probably the right way to go with this. Also, I'm excited to build a plugin system, so there's that :)

What if we have filtering with confirmation by default (somewhat along the lines of @pspeter's solution), and offer an interactive deletion plugin. Anyone that doesn't want to use it doesn't have to.

[wren]

Yup, sounds good! We can have a --delete option (no gui) with a prompt to confirm, with a --force (no one-letter option for now).

[alichtman]

TODO

1. The --force option hasn't been implemented yet.
2. Write tests for --force
3. Fix tests for --delete with mocking

[wren]

We can split the --force flag into a separate PR, if you'd like.

[alichtman]

Yeah, let’s do that.

…

On Mon, Dec 16, 2019 at 5:01 PM Jonathan Wren ***@***.***> wrote: We can split the --force flag into a separate PR, if you'd like. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#698?email_source=notifications&email_token=AE5FN5J6XHUA5GESNEN7VDLQY6Q6FA5CNFSM4JGCR3G2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG7F4TI#issuecomment-566124109>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE5FN5PANAVSHQ6YN5ZQ35DQY6Q6FANCNFSM4JGCR3GQ> .

[wren]

@alichtman Hi! This PR still looks really promising. Are you planning on coming back to it? Or would you like someone else to pick it up?

[alichtman]

If someone else could take it from where I left off, that'd be great. I've got a lot on my plate at the moment and can't fit this in, but this feature definitely deserves to get finished.

[wren]

@alichtman No worries. Thanks for taking it this far!

[dbxnr]

@alichtman Would you be interested in collaborating on a fork of the project? This seems like a really good feature and I'd like to do some more work on it with you

[wren]

@dbxnr Hi! As this is an open-source project, you're well within your rights to fork the project and do what you'd like with it. But please don't use our project's space to recruit for--or advertise--your other project. It's distracting for us here, and bad open-source community etiquette.

[alichtman]

This seems like a really good feature and I'd like to do some more work on it with you

@dbxnr, if you're interested in improving this feature, I'd recommend opening another PR (since this one was just merged).