Skip to content

Conversation

@scottnemes
Copy link
Contributor

Description

This PR creates a new dataclass to store SQL/command results instead of passing around tuples. This should make further code improvements / extensions easier to maintain.

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).
  • I ran uv run ruff check && uv run ruff format && uv run mypy --install-types . to lint and format the code.

@scottnemes
Copy link
Contributor Author

@rolandwalker Let me know what you think! Went better than expected but not sure what you were expecting. Once this goes in I'll update the watch query PR; should hopefully be easier with this.

@dataclass
class SQLResult:
title: str | None = None
cursor: Cursor | list[tuple] | None = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels wrong to call this property cursor when it can also hold a list. What would be a better name? result? Or is that repetitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two options here:

  1. Change the name to "results" and leave everything as is, with the value being overloaded; used as a Cursor sometimes, a list[tuple] other times in the code.

  2. Do another full refactor now to split it into two fields on SQLResult, cursor and results. Then update all the downstream logic that uses the old field to use the correct new field. This will be a fairly big lift as there is a lot of random logic that uses the overloaded field currently. Should be done at some point either way.

I'm fine with either depending on your vision and what you'd like to happen now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1, with some name like "results" or "contents", at your choice.

@rolandwalker rolandwalker merged commit 0553203 into dbcli:main Jan 10, 2026
8 checks passed
@rolandwalker
Copy link
Contributor

Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants