Skip to content

Refactor: Add shared base classes for Cursor and Connection - #842

Open
bgunebakan wants to merge 6 commits into
mainfrom
bilal/refactor-add-shared-base-classes-for-cursor-and-connection
Open

bgunebakan wants to merge 6 commits into
mainfrom
bilal/refactor-add-shared-base-classes-for-cursor-and-connection

Conversation

@bgunebakan

Copy link
Copy Markdown
Contributor

Part of crate/ecosystem#54.

Summary of the changes / Why this is an improvement

Splits Cursor and Connection into a shared base class and the sync class, the planned async client can reuse the parts that do no IO.

  • CursorBase in crate/client/_cursor_base.py holds the cursor state
  • ConnectionBase in crate/client/_connection_base.py holds the converter and time zone, the closed state
  • Cursor and Connection keep all their public methods, signatures and docstrings.

This is a pure refactor with no behavior change, and no changelog entry because nothing changes for users.

Two internal details changed, they are not visible from public API.

  • Cursor.fetchone(), fetchmany() and fetchall() no longer call self.next(), so a subclass that overrides next() no longer changes them.
  • Connection._closed is now set before the server-version probe instead of after it.

Note to reviewer

The PR has 5 commits, and each one passes the tests on its own.

Checklist

@bgunebakan bgunebakan self-assigned this Oct 19, 2026
@bgunebakan bgunebakan added enhancement python Pull requests that update Python code labels Oct 19, 2026
Comment thread src/crate/client/cursor.py Dismissed

@mfussenegger mfussenegger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit hard to judge the abstraction given that there's only one implementation.

My gut feeling is that some of these base methods could be converted to pure methods instead. Therefore my suggestion would be to first add the async variants in addition to the sync variant, and then refactor to extract common logic.

Aside from that, I think some more discussion about the points I brought up in https://github.com/crate/ecosystem/issues/54#issuecomment-6267321475 would be nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants