Skip to content

Conversation

@scottnemes
Copy link
Contributor

Description

Initial goal of the SQLResult dataclass was to be able to use it without having to add all new attributes (i.e. command) to the for loops of every section of the code that uses the basic title/cursor/headers/status fields. This PR reworks the initial implementation to meet that goal.

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 Had a moment of (I think) insight on how to meet my initial goal for this, which was to not have to use all SQLResult fields (i.e. command) in the for loops as that would get out of hand quick on future additions. This also makes things a bit more consistent. Let me know if you see any problems with it though, however it appears to work as I would expect anyway. Didn't update the changelog, as if this gets included it can just be part of the initial dataclass change anyway.

command = {
"name": "watch",
"seconds": seconds,
"seconds": str(seconds),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why str() here and float() later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the weeds on trying to make mypy happy. I reworked it a bit to be more explicit and found a combo mypy liked, so let me know if that's any better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better!

@rolandwalker
Copy link
Contributor

I would work toward not needing get_output() at all and using the dataclass properties directly. But it is good for now!

@rolandwalker rolandwalker merged commit fc4ce84 into dbcli:main Jan 12, 2026
8 checks passed
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