Skip to content

Cityinfo#5

Open
Najipian wants to merge 26 commits intocamelcasetechsd:masterfrom
Najipian:cityinfo
Open

Cityinfo#5
Najipian wants to merge 26 commits intocamelcasetechsd:masterfrom
Najipian:cityinfo

Conversation

@Najipian
Copy link
Contributor

No description provided.

@Najipian Najipian closed this Nov 22, 2018
@Najipian Najipian reopened this Nov 22, 2018
@zhibek zhibek changed the title Cityinfo in-progress Cityinfo Nov 22, 2018
Copy link
Member

@zhibek zhibek left a comment

Choose a reason for hiding this comment

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

@Najipian I'm really happy with the logic you've implemented - well done!

Please check the small changes I've requested.

<body>
<h1>Cities Information</h1>
<dl>
{% for city in cities %}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a condition to be displayed if we don't have data on any cities.


def StoreCitiesInto(cities):
for city in cities:
entity_key = ndb.Key('CityInfo', city['Location'])
Copy link
Member

Choose a reason for hiding this comment

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

Please check indentation.

logging.info("CityInfoRootPipeline")

# Read cityinfo.yaml file
yaml_path = os.path.join(os.path.dirname(__file__), '../cityinfo/cityinfo.yaml')
Copy link
Member

Choose a reason for hiding this comment

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

Please abstract this implemenation into a separate function.


return None

def StoreCitiesInto(cities):
Copy link
Member

Choose a reason for hiding this comment

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

Spelling mistake - StoreCitiesInfo

deploy-cron.sh Outdated
@@ -0,0 +1,20 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

This is good. But to keep things simple, let's remove this deploy-cron.sh file and include the logic in deploy.sh instead (in deploy.sh, we don't need to copy all of this file in, we just need to change the gcloud app deploy app-deploy.yaml line to include cron.yaml after app-deploy.yaml (space separated list).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants