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

Use engine as-is in SqlCreds.from_engine #44

Open
ispedals opened this issue Oct 29, 2021 · 2 comments
Open

Use engine as-is in SqlCreds.from_engine #44

ispedals opened this issue Oct 29, 2021 · 2 comments

Comments

@ispedals
Copy link

I was having similar issues as #23 where the connection string I used to create the SQLAlchemy engine was not in the format from_engine() expected.

However, is there a reason why from_engine() doesn't use the provided engine instance, like so?

def from_engine(cls, engine: sa.engine.base.Engine) -> "SqlCreds":
     sql_creds = cls.__new__(cls)
     sql_creds.engine=engine
     return engine

This prevents any assumptions from being broken, and other than for __repr__(), it doesn't appear the other class attributes are ever used.

@yehoshuadimarsky
Copy link
Owner

This is a very good point. Maybe we can change the engine to only require what it actually uses in the BCP command (server, database, username, password) and use duck typing for that, i.e. if it has those properties than we don't really care about what it is or anything else.

@vlasvlasvlas
Copy link

@ispedals can you make a fork with this change so we can test it? thanks!

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

No branches or pull requests

3 participants