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

Issue 249 postponed configuration #251

Closed
wants to merge 6 commits into from
Closed

Issue 249 postponed configuration #251

wants to merge 6 commits into from

Conversation

artslob
Copy link

@artslob artslob commented Oct 10, 2020

Closes #249

Copy link
Contributor

@vmarkovtsev vmarkovtsev left a comment

Choose a reason for hiding this comment

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

This is a big change that touches some of the most critical code and complicates the object lifecycle considerably. I am not even sure we want the deferred configuration in the first place.

@artslob
Copy link
Author

artslob commented Oct 19, 2020

Yeah, PR and issue are open for discussion to determine whether this PR will be useful. I just think that sometimes its not convenient nor beautiful to parse config at import time. I prefer factory pattern so made this PR.

@rafalp
Copy link
Member

rafalp commented Oct 19, 2020

IMHO there's problem with our API design, but band-aiding it will not help, only make the problem harder to deal with later.

Core object wraps the process of strategy creation, but there's no way to extend this process without modifying internals themselves. This bites not only people who need to configure backend-specific extra options not supported by the core, but also those interested in developing support for other databases.

@artslob
Copy link
Author

artslob commented Oct 25, 2020

Can I do something to improve this PR? Or should I close it then?

@artslob
Copy link
Author

artslob commented Dec 2, 2020

Maybe create some proxy/wrapper around Database class? This way Database would not be changed:

class DatabaseWrapper:
    def __init__(self):
        self.db = None

    def configure(self, **all_rapams):
        self.db = Database(**all_rapams)

    def check_configured(self):
        if not self.db:
            raise ValueError

    def connect(self):
        # proxies all methods to inner object
        self.check_configured()
        return self.db.connect()


class Database:
    # this class would not be changed
    pass


db = DatabaseWrapper()


def main():
    params = parse_env_params()
    db.configure(**params)
    connect = db.connect()

@artslob artslob closed this Aug 2, 2022
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.

Postponed configuration of database
3 participants