support for the provinces and regions of Italy - source: https://osmi… #142
Conversation
|
@napo Please run |
There was a problem hiding this comment.
Hi @napo ! I now did a review for your PR. In general, it looks good 👍🏻 , but there are some changes that are needed. The biggest change that I think would be needed is to remove the dependency of the external files for region and province names.
| from pyrosm.data.bbbike import Cities | ||
| import warnings | ||
|
|
||
| __all__ = ["available", "get_data", "get_path"] | ||
| _module_path = os.path.dirname(__file__) | ||
| _package_files = {"test_pbf": "test.osm.pbf", "helsinki_pbf": "Helsinki.osm.pbf"} | ||
| _package_files = {"test_pbf": "test", "helsinki_pbf": "Helsinki"} |
There was a problem hiding this comment.
These should be as they were, i.e. "test.osm.pbf" and "Helsinki.osm.pbf". These files are bundled with pyrosm.
|
|
||
| # Static test data | ||
| _helsinki_region_pbf = { | ||
| "name": "Helsinki_region.osm.pbf", | ||
| "name": "Helsinki_region", |
There was a problem hiding this comment.
Also this should be "Helsinki_region.osm.pbf". Is there a specific reason why you changed these? Just trying to understand. 🙂
| "url": "https://gist.github.com/HTenkanen/" | ||
| "02dcfce32d447e65024d93d39ddb1812/" | ||
| "raw/5fe7ffb625f091591d8c29128a9e3b37870a5012/" | ||
| "Helsinki_region.osm.pbf", | ||
| "Helsinki_region", |
There was a problem hiding this comment.
Should be "Helsinki_region.osm.pbf"
| ## read the data for the regions | ||
| r = urllib.request.urlopen(italian_regions_json_url) | ||
| data_json = r.read() | ||
| encoding = r.info().get_content_charset('utf-8') | ||
| json_italian_regions = json.loads(data_json.decode(encoding)) | ||
| data_italian_regions = json_italian_regions['objects']['limits_IT_regions']['geometries'] | ||
| italian_resources = {} | ||
| italian_regions = [] | ||
| for r in data_italian_regions: | ||
| name = r['properties']['name'].replace("/","-") | ||
| source = r['properties']['name'].lower().replace(" ","_").replace("'","_").replace("/","-") | ||
| istat = r['properties']['istat'] | ||
| filename = istat + "_" + name | ||
| italian_resources[source] = filename | ||
| italian_regions.append(source) | ||
|
|
||
| ## read the data for the provinces | ||
| r = urllib.request.urlopen(italian_provinces_json_url) | ||
| data_json = r.read() | ||
| encoding = r.info().get_content_charset('utf-8') | ||
| json_italian_provinces = json.loads(data_json.decode(encoding)) | ||
| data_italian_provinces = json_italian_provinces['objects']['limits_IT_provinces']['geometries'] | ||
| italian_provinces = [] | ||
| for r in data_italian_provinces: | ||
| name = r['properties']['name'].replace("/","-") | ||
| source = "provincia di " + r['properties']['name'].lower().replace(" ","_").replace("'","_").replace("/","-") | ||
| istat = r['properties']['istat'] | ||
| filename = istat + "_" + name | ||
| italian_resources[source] = filename | ||
| italian_provinces.append(source) |
There was a problem hiding this comment.
I assume here you are reading the source URLs for the datasets? These lines are likely to be executed every time when something is imported using get_data() which can be problematic. I would prefer storing the italian_regions and italian_provinces as a list of items to avoid parsing something from the web. I assume those names don't change very often?
| italian_provinces.append(source) | ||
|
|
||
| class ItalianProvinces: | ||
| available = italian_provinces |
There was a problem hiding this comment.
See previous comment: I would prefer listing the names here (i.e. hardcode them) to avoid parsing the relatively stable list of names from the web.
|
|
||
| class ItalianRegions: | ||
| #regions = italian_regions | ||
| italian_provinces = ItalianProvinces() |
There was a problem hiding this comment.
See previous comment: I would prefer listing the names here (i.e. hardcode them) to avoid parsing the relatively stable list of names from the web.
| italian_regions_json_url = "https://raw.githubusercontent.com/GISdevio/estratti_OSM_Italia/main/webapp/public/static/boundaries/limits_IT_regions.json" | ||
| italian_provinces_json_url = "https://raw.githubusercontent.com/GISdevio/estratti_OSM_Italia/main/webapp/public/static/boundaries/limits_IT_provinces.json" |
There was a problem hiding this comment.
Are these files "secure"? If downloading something from the web that influence the behavior of the software, I would prefer keeping such files under "good control" i.e. preferably under a user account of pyrosm maintainers. In this way, we can minimize risks that e.g. these files stops existing at some point, or there will be some changes that break something.
There was a problem hiding this comment.
Preferably the list of region and province names should be hard coded to the pyrosm. See comments below.
|
Hi @napo, let me know if you need any further help with this? 🙂 |
…t-estratti.wmcloud.org/w
Edit: