-
Notifications
You must be signed in to change notification settings - Fork 823
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
openstreetmap-carto project file for TagInfo #3151
Conversation
…yle"; "project.mml"
I'm against adding anything to do by hand, especially if it could be easily automated. I still don't have the idea how to make it efficiently after each release, though. I think that while we can include this script, as it is closely related to our code, running this tool is another story. |
I need to re-think, but you can expect some extra documentation line ... Testingtemporary "taginfo-project" test
please help:
|
Sorry, I've made a mistake. It's OK to make the style more strict and consistent (it might be even preferable). I meant RELEASES.md, which you didn't even mentioned... Still it means that this script should be triggered somehow and the best moment seems to be release time. |
|
||
# ------------------------------------------------------------------------------- | ||
# | ||
# This code generate a taginfo project list file (see more https://wiki.openstreetmap.org/wiki/Taginfo/Projects ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate->generates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, fixed!
I think JSON file should not be included in the final version - it will be automatically generated only when needed. It should be always listed in |
I don't think I've understood all that, According to the "Taginfo/Projects/Integration into Taginfo" And I have planned the project file should be here : And I have not added a timestamp inside the JSON, so my |
I guess this file will be created by Travis on next merge, isn't it? |
The script and documentation should be placed inside the |
I am using the Travis only for checking.
if you not need, you can remove. imho: some human checking is useful because my code is not well tested. ( code parsing is hard )
ok, no problem. like this?
imho: than better if we move the taginfo JSON project file to the root. ( so no need a special folder ) my suggestion:
The taginfo expecting a "static/fix url" for the project file, so it can be any url - as you like. |
Yes, this is what I meant.
This would be proper location for a JSON file IMO, if we want to store it in this repo. However I'm still not sure how do you want to run the script? |
We can not access travis files, so Travis can not generate the really used files. Ref #3012 |
what is the alternatives? |
Imo a maintainer should generate the chicago preview png and this json file locally and commits both to this repo if there is a major change |
I said before that I don't like to make more manual steps if they can be done automatically - releasing is already quite complicated and checking the script output differences would be even worse. This script could be run from some other place and I guess even automatically checking the new release is possible, but as simple thing as running it every night for example would do too. |
I have finished. Ready to final review, merge, adapt, delete, reject, modify ... as you like
Sorry, I can't help on this topic, |
Thanks. Unfortunately I can't do the merge here, since I'm not even a programmer. Some coder has to do the code review. Anybody willing? |
@matthijsmelissen What do you think about this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a partial review of the code, and found it difficult to follow. More comments explaining what's going on would help, as well as a clearer flow between functions.
Document your functions with docstrings explaining what they do and what they return.
I am against any approach that requires an additional step during releases, or checking in generated files. When project.mml was generated JSON it was a big headache.
The first step might be to run this script yourself and make the results accessible to others.
osmtype='area' | ||
elif 'planet_osm_line' in ds_table.lower(): | ||
osmtype='way' | ||
elif 'planet_osm_ways' in ds_table.lower(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the ways table, and can't without a schema change.
osmtype='way' | ||
elif 'planet_osm_ways' in ds_table.lower(): | ||
osmtype='way' | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
planet_osm_roads hasn't been processed
|
||
with open( cartocss_project_file , 'r') as f: | ||
newf = yaml.load(f.read()) | ||
f.closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the with block make sure it's closed?
scripts/taginfo-project/README.md
Outdated
* `tags @> '"generator:source"=>wind'` | ||
* `tags @> 'capital=>yes'"]` | ||
* `tags -> 'leaf_type'` | ||
* `tags -> 'wetland'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already an example of this type one line up
# tags -> ARRAY['wheelchair',ramp:wheelchair'] | ||
# tags ?& ARRAY['wheelchair',ramp:wheelchair'] | ||
# tags ?| ARRAY['wheelchair',ramp:wheelchair'] | ||
re_tags_array = re.compile( r"[^a-zA-Z0-9_]tags\s*[@\?-][>&\|]\s*[aA][rR][rR][aA][yY]\[.+?\]" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use re.IGNORECASE as a flag for re.compile to simplify the expression
@@ -1,22 +1,32 @@ | |||
language: node_js | |||
dist: trusty | |||
sudo: false | |||
sudo: required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have a significant impact on how long it takes us to get a Travis instance
.travis.yml
Outdated
- npm install carto@$CARTO | ||
- mkdir -p data/world_boundaries data/simplified-land-polygons-complete-3857 data/ne_110m_admin_0_boundary_lines_land data/land-polygons-split-3857 | ||
- touch data/simplified-land-polygons-complete-3857/simplified_land_polygons.shp data/ne_110m_admin_0_boundary_lines_land/ne_110m_admin_0_boundary_lines_land.shp data/land-polygons-split-3857/land_polygons.shp | ||
script: | ||
# We're using pipes in the checks, so fail if any part fails | ||
- set -o pipefail | ||
# Check the Taginfo-project | ||
- cd ./scripts/taginfo-project && ./generate-taginfo-project-file.py && cd ../.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be callable as scripts/taginfo-project/generate-taginfo-project-file.py
.travis.yml
Outdated
- npm install carto@$CARTO | ||
- mkdir -p data/world_boundaries data/simplified-land-polygons-complete-3857 data/ne_110m_admin_0_boundary_lines_land data/land-polygons-split-3857 | ||
- touch data/simplified-land-polygons-complete-3857/simplified_land_polygons.shp data/ne_110m_admin_0_boundary_lines_land/ne_110m_admin_0_boundary_lines_land.shp data/land-polygons-split-3857/land_polygons.shp | ||
script: | ||
# We're using pipes in the checks, so fail if any part fails | ||
- set -o pipefail | ||
# Check the Taginfo-project | ||
- cd ./scripts/taginfo-project && ./generate-taginfo-project-file.py && cd ../.. | ||
- git diff taginfo-openstreetmap-carto.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to generate to a different file and diff the existing taginfo-openstreetmap-carto.json against the new one, or have the script write to stdout and use <()
Thanks for proposing this, @ImreSamu. The thing I'm worrying about is that this adds fairly complicated code that will break easily under many unsuspected code. Also it is quite a lot of code that achieves relatively little (it only parses the keys). Moreover, checking whether a key is used in the mml does not give any guarantees that the key is actually displayed. So I doubt this is the right way to go. I'm again inclined now towards manually maintaining a list of interpreted tags (which might also serve as a way of specification for us). |
Thank you for you feedback. Imho: My proposal is not guaranteed to be optimal or perfect, but sufficient for my immediate goals. Just creating a "minimal" taginfo project file with a minimal "business value" for the OSM community. As I understand and accept that the maintainence problem is not optimal for my "toy code", Now I am testing my code with other carto forks ( osmfr-cartocss, openstreetmap-carto-hiking-cz, ... ) From the "minimal" taginfo project files I can create some new "business value" :
My proposal is just a temporary ( quick&dirty ) solution. It is not perfect and I accept that it has no 'business value' for the current project members. |
STATUS (2018May08) : As I understand and accept that the maintainence problem is not optimal for my "toy code", so I will move my code to the separate repo/project, and I will close this pull requests in the next days.
background: #961
This is a minimalistic approach: only the osm keys parsed and reported.
I have created a separated directory
./taginfo-project/
New files:
./taginfo-project/README.md
: basic readme./taginfo-project/generate-taginfo-project-file.py
./taginfo-project/taginfo-openstreetmap-carto.json
-> this is the result , for the taginfo!Status:
tags @> ARRAY['building',...]
)(https://github.com/gravitystorm/openstreetmap-carto/blob/master/CONTRIBUTING.md#sql-style-guidelines)