Skip to content
This repository has been archived by the owner on Jan 22, 2021. It is now read-only.

Update the Web API after the data/table/tasks refactoring #26

Merged
merged 10 commits into from
May 28, 2018

Conversation

garaud
Copy link
Owner

@garaud garaud commented May 25, 2018

depend on #24

  • fix several controller functions
  • add unit test to test the API

@garaud garaud requested a review from delhomer May 25, 2018 10:31
@garaud garaud force-pushed the dag-update-webapi branch from b3978ea to 5a69ab5 Compare May 25, 2018 13:03
garaud added 3 commits May 26, 2018 10:51
pytest, ipython, etc.

just do:

   pip install -e ."[dev]"

to install these dependencies.
@garaud garaud force-pushed the dag-update-webapi branch from d5bf361 to e57fde3 Compare May 26, 2018 08:52
@garaud garaud changed the base branch from factorize_tasks to master May 26, 2018 08:55
garaud added 6 commits May 26, 2018 10:56
After the data and tasks refactoring, the table names have beed modified.

* update some web api unit tests
* remove useless functions which were so city-specific
* remove the useless suffix '_route'
* some renaming
* remove unused variables
* rename 'nom' to 'name'
* update the dedicated unit test
@garaud garaud force-pushed the dag-update-webapi branch from e57fde3 to 3829997 Compare May 26, 2018 08:56
Copy link
Collaborator

@delhomer delhomer left a comment

Choose a reason for hiding this comment

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

Nice PR! Good for me, except a specific point: I guess it will be better to fix a few occurrences of unautomated table names. We should refer to {city}.stations and {city}.timeseries from config["database"] configuration section.

,nb_stations as nb_bikes
,st_x(geom) as x
,st_y(geom) as y
FROM {schema}.stations
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could recover the stations table name from config file, as in the previous PR.

FROM {schema}.daily_transaction AS X
LEFT JOIN {schema}.{table} AS Y ON X.id=Y.{idcol}::int
LEFT JOIN {schema}.stations AS Y using(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark as previously.

FROM station AS S
LEFT JOIN {schema}.daily_transaction AS D ON (S.id=D.id)
LEFT JOIN {schema}.{table} AS Y ON S.id=Y.{idcol}::int
LEFT JOIN {schema}.stations AS Y ON S.id=Y.id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idem

query = """SELECT T.*
,S.name as name
FROM {schema}.timeseries AS T
LEFT JOIN {schema}.stations AS S using(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two new occurrences of table names there (timeseries and stations).

table, idcol = station_city_table(city)
query = ("SELECT {id} FROM {schema}.{table}"
";").format(id=idcol, schema=config[city]["schema"], table=table)
query = ("SELECT id FROM {schema}.stations"
Copy link
Collaborator

Choose a reason for hiding this comment

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

stations table could be automatized as well.

"citystation.geom AS geom, "
"rank() OVER (ORDER BY stop DESC) AS rank "
"FROM {schema}.{cluster} AS cs "
"JOIN {schema}.{table} AS citystation "
"ON citystation.{idcol} = cs.station_id::varchar "
"JOIN {schema}.stations AS citystation "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here comes a new occurrence.

@garaud
Copy link
Owner Author

garaud commented May 28, 2018

thanks for the review. I updated the controller.py with your remarks.

@delhomer
Copy link
Collaborator

Fine for me, ready for merging!

@garaud garaud merged commit 980e9ec into master May 28, 2018
@garaud garaud deleted the dag-update-webapi branch May 28, 2018 11:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants