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

Implemented late initialisation for the Manager #31

Closed
wants to merge 2 commits into from
Closed

Implemented late initialisation for the Manager #31

wants to merge 2 commits into from

Conversation

Islati
Copy link

@Islati Islati commented Jul 6, 2016

I stumbled across Alchy after having used SqlAlchemy with Flask-Sqlalchemy and wanted something similar for a non-flask project through work.

The only issue I found, was having a 'late-bind' sort of feature, where in Flask-Sqlalchemy there is the 'init_app()' method.

This is my take on that approach, in a much more straightforward, and direct approach.

It changes nothing under the hood, and assuming it's used responsibly it's a painless feature.

If there's anything I can do to improve this, or further solidify it then please let me know!

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e8981ac on TechnicalBro:feature/late-initialization into 34d19bd on dgilland:develop.

# declaration (i.e: Manager()) then init it
# otherwise, it's a late bind.
if config is not None or session_options is not None \
or Model is not None or session_class is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

I would say that we probably only want to look at config to determine if setup should proceed. While Model will typically be passed in, having Model=None is a supported use-case. The other arguments, session_options and session_class both have sane defaults and may not be provided.

@dgilland
Copy link
Owner

dgilland commented Jul 6, 2016

I'm fine with including this functionality (after code comments addressed). However, we may want to consider throwing an exception in Manager.__getattr__ if Manager.session is None (otherwise, there will be an attribute error since Manager.__getattr__ delegates to Manager.session.

The only other downside I see if the use-case where no config is provided in order to use the default parameter for SQLALCHEMY_DATABASE_URI as 'sqlite://'. Obviously, late-binding would be a breaking change for that use-case but I really have no idea if config=None is used much, if at all (so mostly just a low-impact breaking change).

I will probably also include late-binding configuration in alchy's successor sqlservice. Development on alchy has mostly gone into maintenance-mode while development on sqlservice is moving along (although slowly).

@Islati
Copy link
Author

Islati commented Jul 7, 2016

Right on man! Didn't even realise you had another project in the works; I'll take a look at it when I get a chance.

As for the changes mentioned, I can do that without a problem.

When I get some time in the next day or so, maybe tonight (depends on if I meet my project goals) I can take it on.

Thanks for the feedback, and for the great project!

@dgilland
Copy link
Owner

dgilland commented Jan 4, 2017

Closing due to inactivity. Feel free to resubmit if you have time to wrap it up.

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.

None yet

3 participants