replacing datetime with arrow #82
Conversation
|
Data is same before and after changes for trajectory and trip table: |
|
Since there is no data in recent 7 days, the table appears blank |
There was a problem hiding this comment.
This has the hallmarks of a hastily slapped together set of changes.
- What is the rationale for some of some of the changes?
- Why are we filtering the trajectory table when working on Need to fix time handling for filter by date-range #73? Is this also a fix for Read only data for the past week by default #51?
- In which case, why are we filtering only the trajectory table?
- Where is the testing done?
Please make sure that PRs that are submitted are clean and actually ready for review.
app_sidebar_collapsible.py
Outdated
| @@ -10,8 +10,9 @@ | |||
| For more details on building multi-page Dash applications, check out the Dash documentation: https://dash.plot.ly/urls | |||
| """ | |||
| import os | |||
| from datetime import date | |||
| from datetime import date, timedelta | |||
There was a problem hiding this comment.
if we are switching to arrow, why do we even need datetime?
app_sidebar_collapsible.py
Outdated
| def update_store_trajectoriess(start_date, end_date): | ||
| #it will only load data for recent 7 days in the begining and user can load necessary data adjusting the date filter. | ||
| if not start_date or not end_date: | ||
| end_date_obj = arrow.now().date() - timedelta(days=arrow.now().weekday()) |
There was a problem hiding this comment.
pages/data.py
Outdated
| def update_store_trajectories(start_date_obj,end_date_obj): | ||
| global store_trajectories | ||
| df = query_trajectories(start_date_obj,end_date_obj) |
There was a problem hiding this comment.
why did this have to be removed from here?
utils/db_utils.py
Outdated
| for col in df.columns: | ||
| if df[col].dtype == 'object': | ||
| df[col] = df[col].apply(str) |
There was a problem hiding this comment.
what is the rationale for this change?
There was a problem hiding this comment.
I have reverted this change as this change is already included in another commit.
fixed issue #73