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.

Sign up for GitHub

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

Jump to bottom

Prevent filtered delete from deleting journal #935

Merged
merged 4 commits into from May 6, 2020
Merged

Prevent filtered delete from deleting journal #935

merged 4 commits into from May 6, 2020

Conversation

micahellison
Copy link
Member

@micahellison micahellison commented May 2, 2020 •
edited

Fixes #932 and adds tests for deletion filter scenarios. Also reactivates delete functionality removed in #934.

Checklist

  • The code change is tested and works locally.
  • Tests pass. Your PR cannot be merged unless tests pass. -- poetry run behave
  • The code passes linting via
    black (consistent code styling). -- poetry run black --check . --verbose --diff
  • The code passes linting via pyflakes
    (logically errors and unused imports). -- poetry run pyflakes jrnl features
  • 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?
    Looks like Fixes #932 #933 is also in progress for the same issue.
  • 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?

@justdoit176 justdoit176 mentioned this pull request May 2, 2020
Closed
7 tasks
@micahellison micahellison changed the title [WIP] Prevent filtered delete from deleting journal Prevent filtered delete from deleting journal May 2, 2020
@micahellison micahellison added the bug Something isn't working label May 2, 2020
justdoit176
justdoit176 reviewed May 3, 2020
View reviewed changes
jrnl/Journal.py Outdated Show resolved Hide resolved
wren
wren requested changes May 5, 2020
View reviewed changes
Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic. So many tests! Thank you!

If you can just do the performance enhancement that's already suggested, I think this should be ready to go.

@micahellison @justdoit176
Use more performant deletion
a60d3f9 
Co-authored-by: Fabio <788002+justdoit176@users.noreply.github.com>
@micahellison micahellison requested a review from wren May 6, 2020 19:26
@wren
Copy link
Member

wren commented May 6, 2020

Note: I'm mot sure why the status hasn't updated on the tests, but clicking through to Travis shows the entire test suite passed.

wren
wren approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧨

@wren wren merged commit e302ecb into jrnl-org:develop May 6, 2020
wren pushed a commit that referenced this pull request Jul 25, 2020
@micahellison
Prevent filtered delete from deleting journal (#935)
d9ebfe8 
* Prevent filtered deletion from deleting entire journal #932 and add lots of deletion tests
* Undo removal of deletion feature
* Use more performant deletion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justdoit176 justdoit176 justdoit176 left review comments

@wren wren wren approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"jrnl --delete something" deletes all journal entries
3 participants
@micahellison @wren @justdoit176