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 warning when non wgs84 geographic crs and optional wgs84 default #167

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vincentsarago
Copy link
Member

closes #165

This PR does:

  • add _geographic_crs private attribute (again)
  • add 2 env variable control:
    • MORECANTILE_IGNORE_NON_WGS84_GEOGRAPHIC: silent warning when geographic CRS is not WGS84
    • MORECANTILE_WGS84_GEOGRAPHIC: make WGS84 the default geographic CRS

@AndrewAnnex any opinions?

@AndrewAnnex
Copy link
Contributor

@vincentsarago for this PR since the TMS spec doesn't require WGS84, I feel like warnings related to WGS84 not being used should only be isolated to areas that actually may require it like GeoJSON in #165 (although I will comment there also, I don't like how RFC7946 is used in practice.. but more on that later in #165 comments). Looking at all the new warnings you need to configure to ignore in the pyproject.toml seems to confirm that to me.

Also this is just a bit of "explicit is better than implicit", like if someone uses NZTM2000Quad they know what they are asking for.

That said warnings are 10e10 more preferable to throwing exceptions. I just think it might be better to have the environment variables be a "opt-in" rather than an "opt-out".

For 99% of users, not issuing warnings when they wouldn't occur anyways is fine, for the 1% that know they aren't using WGS84 in their requests a warning about it doesn't help them. But if someone wants to be warned then I think they can enable to config.

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.

2 participants