feat: Make SQLConnector _connect function public

[visch]

Feature scope

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

Description

The function here

sdk/singer_sdk/connectors/sql.py

Lines 82 to 85 in 0285704

	@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

• https://github.com/MeltanoLabs/tap-postgres/blob/5723435f8646f799edff51099fd474b97afb19df/tap_postgres/client.py#L305
• https://github.com/MeltanoLabs/tap-mysql/blob/c84d384ac8cddb2defbf1889128ba1bbdadf3549/tap_mysql/client.py#L68

[edgarrmondragon]

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]

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.

sdk/singer_sdk/streams/sql.py

Lines 209 to 217 in 0285704

	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]

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

[stale]

This has been marked as stale because it is unassigned, and has not had recent activity. It will be closed after 21 days if no further activity occurs. If this should never go stale, please add the evergreen label, or request that it be added.