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

add osm constants #3207

Closed
wants to merge 4 commits into from
Closed

add osm constants #3207

wants to merge 4 commits into from

Conversation

jvonau
Copy link
Contributor

@jvonau jvonau commented May 2, 2022

Fixes bug:

Description of changes proposed in this pull request:

Smoke-tested on which OS or OS's:

Mention a team member @username e.g. to help with code review:

@tim-moody
Copy link
Contributor

adm_const.py.j2 uses
map_doc_root = '{{ vector_map_path }}'

viewer_path is too generic, suggest osm_viewer_path
same for sat_dir, which is really tiles dir
osm_dir looks to b a replacement for vector_map_idx_dir, which is a better name, and shouldn't the value be vector_map_idx_dir = '/library/www/html/common/assets'?

@tim-moody
Copy link
Contributor

one other comment is that I originally put the addition of these constants and functions in 2-common so that they would be available to any subsequent role. that may no longer be the case.

@jvonau
Copy link
Contributor Author

jvonau commented May 2, 2022

adm_const.py.j2 uses map_doc_root = '{{ vector_map_path }}'

That is better, I'll change this PR.

viewer_path is too generic, suggest osm_viewer_path same for sat_dir, which is really tiles dir osm_dir looks to b a replacement for vector_map_idx_dir, which is a better name,

I didn't try to improve on the names used, just the location where these are defined. The names used in maps could of been changed at anytime in the past but has not been but now it's an issue. Suppose the above files in maps could do the translation from the above suggestion to what is used in the scripts. I'll pass on the renaming for now, until others weigh in thanks.

and shouldn't the value be vector_map_idx_dir = '/library/www/html/common/assets'?

Would that not be the same path as https://github.com/iiab/iiab/blob/master/roles/pylibs/templates/iiab_const.py.j2#L18?

@holta holta added this to the 8.0 milestone May 2, 2022
@holta
Copy link
Member

holta commented May 2, 2022

Awesome:

A bit of consolidation after half a decade is a really great idea, especially if this tightens up disparate code for readability!

@holta
Copy link
Member

holta commented May 2, 2022

ASIDE: Overloading and abusing the acronym/name "osm" is probably ok, within limits!

(e.g. sometimes we talk about things like Sentinel satellite photos as if they are OSM, when they really have little to do with OpenStreetMap. But it's hard to resist the 3-letter acronym "osm" even when it's not always entirely accurate...and maybe that's ok ;-)

@jvonau
Copy link
Contributor Author

jvonau commented May 2, 2022

What is the preference path or dir? For full context see https://github.com/iiab/maps/pull/61/files with the proposed names noted.

@holta
Copy link
Member

holta commented May 2, 2022

What is the preference path or dir? For full context see https://github.com/iiab/maps/pull/61/files with the proposed names noted.

Either seems absolutely fine. If it matters:

  • dir is fewer letters
  • path clarifies that it's a full path (not just a relative directory) if that's indeed the case?

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.

None yet

3 participants