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

Propagate kwargs from SessionFactory constructor to create_engine #76

Closed
lincrosenbach opened this issue Mar 12, 2019 · 8 comments
Closed

Comments

@lincrosenbach
Copy link

Kwargs is created empty for setting up the sqlalchemy create_engine this makes impossible to someone add specific settings for creating the engine.
An alternative would be to propagate from session factory kwargs. By doing that, it should allow any user to set all possible parameters like in:
https://docs.sqlalchemy.org/en/latest/core/engines.html#engine-creation-api

Issue reference:

@siddhantgoel
Copy link
Owner

I've actually been thinking of removing the custom arguments that SessionFactory defines (hence the warning when session_events and engine_events are present) and just defining the constructor as def __init__(self, *args, **kwargs) and then passing those arguments directly to SQLAlchemy's create_engine. Thoughts?

@lincrosenbach
Copy link
Author

I like your proposed change. It would make more flexible but maybe backward compatibility may be compromised. Any use of it with positional arguments may break the API. Let me see if our colleagues have some thoughts about it. @igorcandeia @nicoddemus @kfasolin @igortg

@nicoddemus
Copy link

I don't see much problem in breaking backward compatibility here: if you are following semantic versioning then there are no backward compatibility guarantees for 0.x versions.

@kfasolin
Copy link

I agree, since the definition on SQLAlchemy already is sqlalchemy.create_engine(*args, **kwargs) letting *args, **kwargs here makes it transparent to configure the engine the way its needed

@siddhantgoel
Copy link
Owner

Cool. I've requested an update to #78 .

@siddhantgoel
Copy link
Owner

Just merged #78 . I'll try to make another release in the next 2-3 days.

@lincrosenbach
Copy link
Author

Solved on #78

@siddhantgoel
Copy link
Owner

Just pushed v0.6.1.

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

4 participants