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

Fix for #1639 journal overwrite #1640

Closed
wants to merge 5 commits into from
Closed

Conversation

richardjs
Copy link
Contributor

This fixes the journal overwrite bug in #1639.

There's a bit of awkward branching to account for existing use cases of calling jrnl --edit on empty journals; if we drop those, the cleaner fix in fce5aff could likely work.

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.

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.

Instead of returning, jrnl should open the editor like any other empty search result.

For instance, if you run jrnl --edit -contain StringThatIsNotInYourJournal and enter an entry, it will save that new entry.

I'd expect that jrnl --edit -contain StringThatIsNotInYourJournal --change-time would behave exactly the same.

@richardjs
Copy link
Contributor Author

@micahellison, ok, I've modified the fix to behave in that way. I assumed we'd then want to set the newly created entry's time to the value passed to --change-time (or the default now). Unfortunately, as the change time operation runs before the edit operation (see here), I needed to add a little bit of a special case to _edit_search_results.

@micahellison
Copy link
Member

Thanks @richardjs. @wren and I are chatting about this and realizing that this need for a special case reveals a bigger problem in the original flow that we'd like to fix. We definitely would like to keep your test, but we'll need some more time to think about the rest of it before considering merging this. We'll plan on getting back into this next week.

@micahellison
Copy link
Member

Hi again @richardjs, just wanted to let you know, we're still working on this, but it's a bit hairier than we thought. We're basically replacing the branching edit/change-time/delete actions with sequential actions, and I think it will be a lot simpler in the long run, but we're not quite there yet.

@micahellison
Copy link
Member

Closing this PR since it's resolved in our larger structural changes in #1669. Thanks for your work on getting this going, @richardjs.

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.

2 participants