Skip to content

Export Value and ParameterValue type aliases #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Export Value and ParameterValue type aliases #79

merged 2 commits into from
Oct 4, 2024

Conversation

jsurany
Copy link

@jsurany jsurany commented Sep 23, 2024

resolves #78

Describe your changes
Value type has been replaced by ColumnValue and ParameterValue types, both of which are exposed via both cdb2.py and dbapi2.py modules. Updated anything that uses the Value type to now use one of those two types.

Testing performed
Currently no tests. I could create a series of tests that look like below, but I'm not sure how value-added that is, since none of the other exports from any modules are similarly tested.

# test_exports.py
def test_column_value_is_exported_from_dbapi2() -> None:
    from comdb2.dbapi2 import ColumnValue

or I could build a test that creates a fake package that imports these types and then runs mypy, but again I'm not sure how value-added that is.

Additional context
Spoke with Matt about this change and he suggested breaking it into the two types, since Value was used for both parameters and for column values returned inside result sets. I've adjusted functions that use Value to use one of the two replacements. We aren't going to similarly expose the Row type since it just aliases Any, and it's not a big DevX burden to just use Any for Row.

@godlygeek
Copy link
Contributor

We aren't going to similarly expose the Row type since it just aliases Any, and it's not a big DevX burden to just use Any for Row.

And more importantly, because exposing Row as an alias for Any would be a commitment that might get in the way of eventually making the Handle and Connection and Cursor types generics that are parametrized by row type.

Spoke with Matt about this change and he suggested breaking it into the two types, since Value was used for both parameters and for column values returned inside result sets.

I'm gonna go back on that a little bit. I'm going to suggest we leave a Value type for backwards compatibility, but use it only for the column values, and add a new ParameterValue that's used for values that can be passed as parameters. I'm gonna make that change.

Update `comdb2.cdb2` and `comdb2.dbapi2` to export 2 `typing.Union` type
aliases:

- `Value`: the types that values representing result set columns may
  have
- `ParameterValue`: the types that bound parameters of a SQL statement
  may have

Signed-off-by: Jonathan Surany <[email protected]>
@godlygeek godlygeek changed the title Value type is now ParameterValue and ColumnValue types, exported Export Value and ParameterValue type aliases Oct 1, 2024
Signed-off-by: Matt Wozniski <[email protected]>
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

After my changes, this LGTM, but I'd appreciate another set of eyes since I changed it pretty heavily.

@sarahmonod Mind giving this a review as well?

@godlygeek godlygeek requested a review from sarahmonod October 1, 2024 23:17
@godlygeek godlygeek self-assigned this Oct 1, 2024
Copy link
Contributor

@sarahmonod sarahmonod left a comment

Choose a reason for hiding this comment

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

LGTM too!

@godlygeek godlygeek merged commit 518dd75 into bloomberg:main Oct 4, 2024
11 checks passed
@jsurany jsurany deleted the expose_value_types branch October 4, 2024 22:57
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.

Expose Value type explicitly
4 participants