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

Split redash/__init__.py to prevent import time side-effects. #3601

Merged
merged 9 commits into from
Apr 18, 2019

Conversation

jezdez
Copy link
Member

@jezdez jezdez commented Mar 18, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Bug Fix

Description

This basically makes sure that when import the redash package we don't accidentally trigger import-time side-effects such as requiring Redis.

Refs #3569 and #3466.

@jezdez jezdez requested a review from arikfr March 18, 2019 09:19
@ghost ghost assigned jezdez Mar 18, 2019
@ghost ghost added the in progress label Mar 18, 2019
@jezdez jezdez mentioned this pull request Mar 18, 2019
3 tasks
from .organization import DATE_FORMAT


def all_settings():
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't needed since Flask has app.config.from_object

from .utils import routes


class Redash(Flask):
Copy link
Member Author

Choose a reason for hiding this comment

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

I basically tried to separate Flask app specific code in the flask.Flask subclass and all Redash specific code in the create_app function.

redash/__init__.py Outdated Show resolved Hide resolved
redash/app.py Show resolved Hide resolved
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Thanks! See comments.

Btw, you could just move the version check into create_app, but making the code tidier is a good thing too :-)

redash/cli/__init__.py Show resolved Hide resolved
redash/settings/__init__.py Show resolved Hide resolved
redash/utils/routes.py Outdated Show resolved Hide resolved
redash/metrics/celery.py Outdated Show resolved Hide resolved
redash/__init__.py Outdated Show resolved Hide resolved
redash/__init__.py Outdated Show resolved Hide resolved
redash/__init__.py Show resolved Hide resolved
@jezdez
Copy link
Member Author

jezdez commented Mar 21, 2019

@arikfr Is there anything else needed to merge this?

@jezdez jezdez requested a review from arikfr March 21, 2019 10:37
@jezdez jezdez modified the milestones: v7.0.0, Next Mar 22, 2019
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Looks good, but let's move the Celery events connections back to decorators.

Thanks.

@jezdez jezdez merged commit aa9d246 into master Apr 18, 2019
@jezdez jezdez deleted the import-side-effects branch April 18, 2019 16:39
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
…ash#3601)

## What type of PR is this? (check all applicable)
<!-- Please leave only what's applicable -->

- [x] Refactor
- [x] Bug Fix

## Description

This basically makes sure that when import the redash package we don't accidentally trigger import-time side-effects such as requiring Redis.

Refs getredash#3569 and getredash#3466.
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.

2 participants