-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Upgrade dashboard interaction issue #1249
Conversation
There seems to be some issues with automatic checks on alembic. I am not entirely sure why. Could this be something to do with database recreation in order to fix the v1 site (Issue #1239 ? ) |
Hey I just reran the test, and it passed successfully. I guess the database wasn't initialized properly in your run. The test brings up a local database, so we wouldn't expect any issues with the prod database to affect it. I'll start reviewing these changes now, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring this out Josh!
I mentioned this in the comments, but Piero is actively working on updating neighborhoods.py in PR #1242. Let's wait for him to merge that before applying the Dash upgrade to that file.
In general, my comments are about Python style. In particular, we should use snake_case
for variable names. We should also organize imports as described here: https://google.github.io/styleguide/pyguide.html#313-imports-formatting.
server/dash/app.py
Outdated
|
||
external_stylesheets = ['/static/reports.css'] | ||
app = dash.Dash(__name__, external_stylesheets=external_stylesheets) | ||
server = flask.Flask(__name__) # define flask app.server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is not informative beyond the code itself, so we can get rid of it.
import textwrap | ||
|
||
import dash_core_components as dcc | ||
import dash_html_components as html | ||
import pandas as pd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, please follow guidelines for import formatting here: https://google.github.io/styleguide/pyguide.html#313-imports-formatting
import textwrap | ||
|
||
import dash_core_components as dcc | ||
import dash_html_components as html | ||
import pandas as pd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just remembered that Piero is updating this file right now. Let's wait until he merges his PR, and then could you pull and update the dash stuff please?
server/dash/index.py
Outdated
from dash.dependencies import Input | ||
from dash.dependencies import Output | ||
from dash import Dash, html, dcc, callback, Input, Output | ||
# from dash.dependencies import Input, Output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
BATCH_SIZE = 10000 | ||
|
||
def batch_get_data(url): | ||
# set up your query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please capitalize the start of all comments, and add a period at the end.
Could you use an autoformatter for Python in your code editor? I think that will help with some of the style issues like spacing. |
Fixes #1246
Any questions? See the getting started guide