Feat/connection block #184
Conversation
pynuodb/connection.py
Outdated
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| # type: () -> None |
There was a problem hiding this comment.
Nit: Spacing is off (one too many spaces).
There was a problem hiding this comment.
Thanks @adriansuarez for catching that! I've fixed the spacing issue in the latest commit.
pynuodb/connection.py
Outdated
| try: | ||
| if exc_type is None: | ||
| # No error: commit if needed | ||
| if not self.autocommit: | ||
| self.commit() | ||
| else: | ||
| # Error! Rollback if needed. | ||
| if not self.autocommit: | ||
| self.rollback() | ||
| finally: |
There was a problem hiding this comment.
This is the crux of the change. Whether we should commit automatically on exit or not.
My personal feeling is that we should not do this. If the user sets autocommit=False and they don't explicitly commit, then we should not commit "for" them... even when there's no exception.
Do we have any examples from other database's python drivers, to see how they handle this situation?
@rshaull do you have thoughts about this?
There was a problem hiding this comment.
There are two common patterns for how a connection context manager behaves:
Commit on exit – as seen in sqlite3, where exiting the with block commits any pending changes.
Rollback on exit – as seen in python-oracledb, where uncommitted changes are rolled back when the context closes.
Since there’s no universal standard, we need to decide which behavior aligns best with our use case.
There was a problem hiding this comment.
Without these changes, if the programmer explicitly closes a connection without an explicit commit or rollback, any uncommitted statements will be rolled back by the database. The same is true if they forget to explicitly close the connection.
We should preserve that behavior. There is no need to explicitly call rollback() when closing the connection. You should not call commit().
There was a problem hiding this comment.
Updated exit_ block behavior without calling rollback or commit explicitly
| @@ -350,3 +350,24 @@ def cursor(self, prepared_statement_cache_size=50): | |||
| """ | |||
| self._check_closed() | |||
| return cursor.Cursor(self.__session, prepared_statement_cache_size) | |||
|
|
|||
| def __enter__(self): | |||
There was a problem hiding this comment.
We're missing a # type comment here.
There was a problem hiding this comment.
Added required comments and docstrings for documentation.
|
We do need to figure out why you're getting test failures. On my system the tests are all passing (for the current master branch HEAD). |
|
As far as I can see this PR didn't trigger a circleci check. Did I miss it? I confess I'm not very familiar with this config, but when I push a change it automatically runs. Maybe we only have it set up to run when changes are made to this repo but not to any forks? @adriansuarez do you know how to configure this? |
I will manually kick off a run of our CircleCI pipeline. We have automatic runs triggered by fork PRs turned off to prevent someone from making a code change that exposes / uploads sensitive data like the NuoDB license, which is exported as an environment variable in the test setup. This change is obviously not doing anything like that. |
|
@Vats-shivam FYI, the latest CI run failed with the error: |
@adriansuarez My bad, really sorry didn't tested last time. |
I have updated the Connection class with entry and exit method as well as removed the contextlib.closing method from the existing test suite below i have attached the test results
This basically involves with connection.py file and nuodb_date_time_test.py file