Skip to content

Pure Julia WKT2 to PROJJSON conversion #156

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Omar-Elrefaei
Copy link

Working towards #150

@Omar-Elrefaei Omar-Elrefaei changed the title WKT2 to Julia Dict to PROJJSON Pure Julia WKT2 to PROJJSON conversion Apr 24, 2025
@juliohm
Copy link
Member

juliohm commented Apr 28, 2025

@Omar-Elrefaei this looks promising. Please let me know when it is ready for review.

@nsajko
Copy link

nsajko commented Apr 28, 2025

Some deep issues with the design here:

  • Calling Meta.parse on the WKT string can't be correct and is obviously a huge hack, no offence. It's parsing foreign code as Julia code!
    • In particular WKT strings have completely different escaping than Julia strings, the former escape by repeating the double quotes, while the latter escape with backslashes.
    • It could also perhaps be considered a security vulnerability, unless the WKT strings are completely vetted upfront on each update of the EPSG data base.
  • WKT keywords are case-insensitive, furthermore in some cases several alternative names are possible for the same keyword, so hardcoding the names like here can't be correct.

My design is more complex in part because it tackles these issues and more.

@juliohm
Copy link
Member

juliohm commented Apr 28, 2025

Thank you for sharing @nsajko. Appreciate your inputs.

If you can evolve your PR to a final version to show the technical advantages in practice, we will happily consider it.

Both PRs are still work in progress, but it is really nice to see the high quality of the attempts already.

@Omar-Elrefaei Omar-Elrefaei marked this pull request as ready for review May 1, 2025 17:27
@Omar-Elrefaei
Copy link
Author

I think this is functionally mostly there, save for few rare edge cases.
The code itself would take another round of cleanup tho, especially the comments.

few of the concerns brought up by nsajko are justifies, some are design tradeoffs, and some are a non-issue, I think. I'll take time to elaborate soon.

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.

3 participants