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

world_bank_datasets #1075

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

swethammkumari
Copy link

@@ -334,7 +348,7 @@ def load_stat_vars(stat_var_file):
else:
logging.error('SKIPPED stat var row: %s', csv_row)

logging.info(svs)
#logging.info(svs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove commented line.

Copy link
Contributor

@shamimansari1988 shamimansari1988 left a comment

Choose a reason for hiding this comment

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

Code looks good, so approved the changes.

Choose a reason for hiding this comment

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

  1. Please add Readme.md file
  2. Update copyright year to 2024

logging.info('Writing response to file %s', response_file)
json.dump(response, f, indent=2)
return response
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

If the download fails for existing data, better to abort the processing as autorefresh with partial data will result in deletions in prod.

Choose a reason for hiding this comment

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

We have one tMCF file and multiple CSV files. So can we add wildcards in CSV Paths instead of referring to each individual CSV file?

Copy link

@krishnaswamypradeep krishnaswamypradeep left a comment

Choose a reason for hiding this comment

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

Please fix the comments

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.

4 participants