Skip to content
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

feat: Make SQLConnector _connect function public #2262

Open
visch opened this issue Feb 22, 2024 · 3 comments
Open

feat: Make SQLConnector _connect function public #2262

visch opened this issue Feb 22, 2024 · 3 comments
Labels
kind/Feature New feature or request SQL Support for SQL taps and targets valuestream/SDK

Comments

@visch
Copy link
Contributor

visch commented Feb 22, 2024

Feature scope

Taps (catalog, state, stream maps, tests, etc.)

Description

The function here

@contextmanager
def _connect(self) -> t.Iterator[sa.engine.Connection]:
with self._engine.connect().execution_options(stream_results=True) as conn:
yield conn

I'd like to change to the name connect as we use this in different taps.

See

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Feb 22, 2024

Thanks @visch!

I think the "private" _connect() method is only an issue if you use it outside of the SQLConnector implementation. For example, here: https://github.com/MeltanoLabs/tap-mysql/blob/c84d384ac8cddb2defbf1889128ba1bbdadf3549/tap_mysql/client.py#L427.

It hints that something should be implemented in MySQLConnector connector.

Note this was introduced in #1394 to ensure there's a single entry point for handling the connection object: #1393.

@visch
Copy link
Contributor Author

visch commented Feb 22, 2024

I'm probably missing something @edgarrmondragon

I think the "private" _connect() method is only an issue if you use it outside of the SQLConnector implementation.

with self.connector._connect() as conn: # noqa: SLF001
for record in conn.execute(query).mappings():
# TODO: Standardize record mapping type
# https://github.com/meltano/sdk/issues/2096
transformed_record = self.post_process(dict(record))
if transformed_record is None:
# Record filtered out during post_process()
continue
yield transformed_record

The SDK does it too.

What triggered this for me is the linter told me I should so I added the code below. It just seemed weird to have to lint for the "standard" use case
https://github.com/MeltanoLabs/tap-mysql/blob/c84d384ac8cddb2defbf1889128ba1bbdadf3549/tap_mysql/client.py#L427

We can just close and I can ignore it 🤷 , it's me being too pedantic

@edgarrmondragon
Copy link
Collaborator

No worries. We might end up doing this anyway if #1649 is shipped, or at least its Connector interface definition.

@edgarrmondragon edgarrmondragon added the SQL Support for SQL taps and targets label Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/Feature New feature or request SQL Support for SQL taps and targets valuestream/SDK
Projects
None yet
Development

No branches or pull requests

2 participants