⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.91%. Comparing base (ff54025) to head (210a746).
Report is 40 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
- Coverage   99.34%   98.91%   -0.43%     
==========================================
  Files           3        3              
  Lines         455      462       +7     
==========================================
+ Hits          452      457       +5     
- Misses          3        5       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

So this registers a cleanup function (sqlite3.Connection.close) when the DB object is garbage collected. The tests are fine, the warning is gone....lgtm

@mbhutton Thank you!

Two minor things, otherwise LGTM.

1. Is the assignment to self._finalizer strictly necessary? According to the docs

Unlike an ordinary weak reference, a finalizer will always survive until the reference object is collected, greatly simplifying lifecycle management.

2. Do you think it might be helpful to add a brief comment above weakref.finalize to explain the context here for future people?

Hi @mikez,

1. Is the assignment to self._finalizer strictly necessary? According to the docs

Unlike an ordinary weak reference, a finalizer will always survive until the reference object is collected, greatly simplifying lifecycle management.

Yes I think so. The only options I can see are either:

• Ignore the warning
• Use a separate connection for each transaction (slower)
• Provide a manual close() method for use by callers, pushing responsibility to close to the API client
• Create a context manager for the Database to automatically close when going out of scope (requiring changes to the caller)
• This approach: clean up on GC. Chosen as the least invasive option which keeps the performance gains.

If there were things.py clients which used large numbers of Database instances then one of the other options would be more appropriate, to force earlier cleanup, but I know this isn't an issue in practice based on the fact that this library has historically used a single connection.

The reference to self is required for the finalizer to work, as well as the connection reference to support the Connection.close() call.

2. Do you think it might be helpful to add a brief comment above weakref.finalize to explain the context here for future people?

I've added the following comment above the finalizer call:

# Close the underlying SQLite connection when this Database object is garbage collected

Does that sound okay?

@mbhutton Quick clarification on point 1. What I meant with referring to that citation in the docs was doing only the function call

weakref.finalize(self, sqlite3.Connection.close, self.connection)

as opposed to the assignment

self._finalizer = weakref.finalize(self, sqlite3.Connection.close, self.connection)

The strategy itself I like.

2. Sounds good to me.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mikez Thanks for clarifying, self._finalizer assignment now removed, much nicer without it!

I believe that's everything resolved?

@mbhutton Looks great to me! Thanks for your work here. 💛