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

Allow combinations of --change-time, --delete, and --edit while correctly counting the number of entries affected #1669

Merged
merged 37 commits into from
Mar 25, 2023

Conversation

wren
Copy link
Member

@wren wren commented Jan 28, 2023

@micahellison
Copy link
Member

This might fix #1666. We'll need to add a test.

@micahellison
Copy link
Member

micahellison commented Feb 11, 2023

We'll need to add some tests for:

  • combinations of action arguments (edit, change-time, delete)
  • combinations of stderr output messaging (entries found, entries modified)
  • might need some tests for 0 vs 1 vs multiple entries found and/or modified
  • unit tests for the new private controller functions

We'll need to come up with a better name for this PR too. It changes some messaging behavior and we'll want that reflected in the changelog.

@micahellison
Copy link
Member

I noticed some problems with incorrect count messages while testing this out today, so I added some failing tests for those issues.

@micahellison
Copy link
Member

This is green but not finished. We still need to add the tests I mentioned in my comment last week.

We also changed the messaging for when you delete all entries in the editor. Instead of the No entry to save, because no text was received message, we're showing this warning:

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃  No text received from editor. Were you trying to delete all the entries?  ┃
┃                                                                            ┃
┃  This seems a bit drastic, so the operation was cancelled.                 ┃
┃                                                                            ┃
┃  To delete all entries, use the --delete option.                           ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

@micahellison
Copy link
Member

I added some failing tests that we should look at. I'm not 100% certain what the added/found/deleted counts should be with certain combinations of actions, but I still think they are wrong now in certain situations.

@micahellison
Copy link
Member

I finally decided to just keep a register of deleted and added entries in the Journal class. We're already tracking modified entries in its member class Entry so this is not a significant deviation, and now the counts reflect exactly what jrnl did to the journal, even when combining all three actions (change-time, delete, and edit).

This change also has a nice side effect of moving all counting logic outside of the controller class, which has no business doing that anyway. In the long run, I'd like to separate this logic from the data representation of the journal, but I think that's outside of the scope of this PR.

@micahellison micahellison changed the title [WIP] Rework how actions are handled in controller [WIP] Allow combinations of --change-time, --delete, and --edit while correctly counting the number of entries affected Mar 1, 2023
@micahellison
Copy link
Member

I still want to add more tests to this. The combination tests are a good start, but I want to test them with search criteria as well.

micahellison and others added 19 commits March 4, 2023 13:58
- streamline `run` function in `controller.py`
- add debug logging
- fix unnecessary import of Journal class (only needed for typing)
- standardize summary display across different actions
…hen no entries found by short-circuiting display method
* Add documentation about using Vim/Neovim as editor
* Add documentation about information leaks in editors
* Spelling fix

---------

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>
wren added 2 commits March 4, 2023 14:07
Conflicts:
  CHANGELOG.md
  docs/privacy-and-security.md
  jrnl/controller.py
  tests/bdd/features/change_time.feature
@micahellison micahellison changed the title [WIP] Allow combinations of --change-time, --delete, and --edit while correctly counting the number of entries affected Allow combinations of --change-time, --delete, and --edit while correctly counting the number of entries affected Mar 25, 2023
@micahellison micahellison added the bug Something isn't working label Mar 25, 2023
@wren wren marked this pull request as ready for review March 25, 2023 19:15
@wren wren merged commit 3c923ae into jrnl-org:develop Mar 25, 2023
@wren wren deleted the mode-actions-1639 branch March 25, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants