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

Fetching GeoJSON file from Pyro-Risk release #39

Merged
merged 4 commits into from
Mar 3, 2021

Conversation

pechouc
Copy link
Collaborator

@pechouc pechouc commented Feb 15, 2021

Hello everyone!

Following #35, here is a very light PR to switch from reading the local GeoJSON file in the data folder to fetching the file online, from a release in the Pyro-Risk repository.

The first commit thus changes:

  • the alerts.py file to (i) move the file reading procedure outside of the build_departments_geojson function, (ii) modify this process from a local reading to the online one and (iii) adapt imports and comments as needed;
  • the risks.py file in which the file reading procedure was already outside of the function;
  • the config.py file, so as to store the URL towards the release in a clean way.

The second commit simply consists in eliminating the unnecessary local file.

My only interrogation is the following. Because the file is the same for both alerts and risks views, we could theoretically fetch the file only once for both, which would be more efficient. But at the same time, in risks.py, we are also fetching the estimated risk scores from the data science team and use them to modify the departments variable. So, it seems that it would require some work to merge both requests to the online file and it feels as a bit of an overkill to me.

What is your opinion on this? Should we absolutely read the file only once for both maps?

@pechouc pechouc added the type: enhancement New feature or request label Feb 15, 2021
@pechouc pechouc self-assigned this Feb 15, 2021
Copy link
Contributor

@chloeskt chloeskt left a comment

Choose a reason for hiding this comment

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

Hi @pechouc ! Thank you for this PR, seems great !!

I've had a look at this issue given by Codacy. This is that there are security issues with urllib, especially due to the permitted schemes (SO thread). I guess that the security risk is not that important in our case, but we should try to have as many best practices as possible. Maybe we should use another library such as requests which do not face the same issue ? or make sure that the transfer protocol is http or https if you want to use urllib ?

@pechouc
Copy link
Collaborator Author

pechouc commented Feb 15, 2021

Thanks for the heads-up, I had missed the Codacy check! I have moved to requests in a new commit, which presents the additional advantage of harmonising these few lines with the process already used for getting the risk score.

Copy link
Contributor

@chloeskt chloeskt left a comment

Choose a reason for hiding this comment

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

That is great !

Merging master following the merge of Adel's Pull Request.
@frgfm
Copy link
Member

frgfm commented Mar 3, 2021

Just updated @chloeskt access to the repo, you now have write access 👌
So @pechouc, you can move ahead with the merge if everything looks alright to you :)

@chloeskt
Copy link
Contributor

chloeskt commented Mar 3, 2021

Thanks @frgfm ☺️

@pechouc
Copy link
Collaborator Author

pechouc commented Mar 3, 2021

Wonderful, thanks a lot!

@pechouc pechouc merged commit 0475f88 into master Mar 3, 2021
@pechouc pechouc deleted the removing_geojson_file branch March 3, 2021 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants