Skip to content
This repository has been archived by the owner on Aug 30, 2024. It is now read-only.

Document.atomic context manager #428

Closed
wants to merge 1 commit into from
Closed

Document.atomic context manager #428

wants to merge 1 commit into from

Conversation

aogier
Copy link
Contributor

@aogier aogier commented Feb 10, 2019

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

This PR implements a Document.atomic context manager that don't save a document if there is any uncatched exception in inner code block. Tentatively fixes #427.

Approach

As this change could be breaking (in fact it change a default behaviour) I've decided to implement a brand new context manager using python's contextlib.
When it's needed, one could use atomic as a context manager:

with Document(self.db, 'julia006').atomic() as doc:

Context Manager implementation follow current one, in fact it uses the same dunders.

Schema & API Changes

  • Added a new Document.atomic context manager

Security and Privacy

  • "No change"

Testing

  • Added new tests:
    • unit.document_tests.test_document_context_manager_atomic_creation_failure
    • unit.document_tests.test_document_context_manager_atomic_update_failure

Monitoring and Logging

  • "No change"

Copy link
Contributor

@smithsz smithsz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@smithsz
Copy link
Contributor

smithsz commented Feb 14, 2019

Thanks again for this. We've decided to change the default behaviour. I've made the change in .__exit__ and stolen your tests (see #434).

@smithsz smithsz closed this Feb 14, 2019
@aogier aogier deleted the feature/atomic-cm branch February 14, 2019 13:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document.Document context manager ignore exceptions
2 participants