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 ProjectionDefinition.title from wkt.AUTHORITY #29

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

ftoromanoff
Copy link
Contributor

Following the DefinitelyTyped/types/proj4 (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/72db0783554789ef424e5dc5a207a028c2a0169f/types/proj4/index.d.ts)
title in ProjectionDefinition should be mandatory but is nowhere to be found in index.cleanWKT().
I just added the fact that in cas ther is an AUTHORITY defined in the wkt that we use this information as title (parsed as a string).

In case i'm not posting in the right place, or i'm not following the way it should be, fell free to tell me where and what I should do.

Thanks

wkt.build.js Outdated
Comment on lines 300 to 303
if (wkt.AUTHORITY) {
var authority = Object.keys(wkt.AUTHORITY)[0];
wkt.title = `${authority}:${wkt.AUTHORITY[authority]}`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is auto-built, you should not be making changes to it. We'll need to make this clear in the README and/or with a proper .gitignore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't think it makes sense to change the library to match its 3rd party type definition. It should be the other way around. Which means @types/proj4 should be changed to match what we have here.

I could be convinced otherwise if there is a strong argument for having a title that shows the authority. I can't find one myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the 3rd party type definition title was the only obligatory property, thus It was the only one on wich I could relied as an existing id, And follwing EPSG.io, I fell like title in format 'EPSG:XXXX' seemed like a good identifier.

What property should I use to id a specifique parsed wkt ?

Copy link
Member

@ahocevar ahocevar Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @types/proj4 types tell fairy tales. title is neither used for wkt nor for proj definitions in proj4js. If you're using wkt-parser standalone, name at the root level is what you should be looking at. As far as I know, the AUTHORITY entry is not mandatory in the WKT v1 spec, so your pull request would cause errors for wkt defs that don't contain one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification about the @types/proj4.
I was using the name field but to avoid doublon between a proj.def defined through it's name and the same one defined using EPSG:XXXX, i thought about trying to get a more unique identifier.

Just to clarify I don't understand how my PR would cause errors as it's only if there is an AUTHORITY field that i set the property title from it.

Copy link
Member

@ahocevar ahocevar Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would cause errors or at least unexpected output if what you're putting in the title string is not available. So I'd prefer something like

if (wkt.AUTHORITY) {
  var authority = Object.keys(wkt.AUTHORITY)[0];
  if (authority && authority in wkt.AUTHORITY) {
    wkt.title = `${authority}:${wkt.AUTHORITY[authority]}`;
  }
}

Copy link
Contributor Author

@ftoromanoff ftoromanoff Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added what you suggested to avoid the case with an empty wkt.AUTHORITY.

@ahocevar ahocevar mentioned this pull request Oct 10, 2024
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ftoromanoff

@ahocevar ahocevar merged commit d61e880 into proj4js:main Oct 11, 2024
1 check passed
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