Introduce WGS84 DefinedNamespace by kvjrhall · Pull Request #1710 · RDFLib/rdflib (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

kvjrhall

The _WGS84 namespace was introduced to the namespace package matching the
convention of other namespaces. The comment style of the GEO vocabulary was
mimicked for consistency. wgs84:geometry was given additional documentation
because it is not a standard term in the namespace. It was included because it
is used by dbpedia. The prefix 'wgs84' was adopted for the namespace so that it
would not conflict with the current usage of 'geo' for GeoSPARQL.

Fixes #1709

Proposed Changes

@kvjrhall

The _WGS84 namespace was introduced to the namespace package matching the convention of other namespaces. The comment style of the GEO vocabulary was mimicked for consistency. wgs84:geometry was given additional documentation because it is not a standard term in the namespace. It was included because it is used by dbpedia. The prefix 'wgs84' was adopted for the namespace so that it would not conflict with the current usage of 'geo' for GeoSPARQL.

@kvjrhall

The more commonly used prefix for wgs84 is wgspos, at least when it's looked up through sites like prefix.cc. It makes sense to use that rather than wgs84 which likely hasn't seen any use.

@kvjrhall

My best guess for a different prefix was wgs84, but at least in prefix.cc the prefix wgspos resolves to the namespace this pull request would introduce. Updated accordingly.

@edmondchuc

@kvjrhall

Edmond Chuc commented that he'd seen the wgs prefix in use but not wgspos. He also noted that it also resolves to wgs84 at http://prefix.cc/wgs. It's shorter/cleaner and shouldn't conflict with anything

@kvjrhall

@kvjrhall

Sold me on it. I also renamed the class WGS84 to WGS so that it matched the prefix per existing convention.

aucampia

@@ -75,6 +75,7 @@
* TIME
* VANN
* VOID
* WGS84

Choose a reason for hiding this comment

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

aucampia

Choose a reason for hiding this comment

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

Looks good other than the docstring (see suggestion).

kvjrhall

Contributor Author

@kvjrhall kvjrhall left a comment • Loading

Choose a reason for hiding this comment

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

Ahh new to reviews on github, trying to find the suggested docstring changes

@kvjrhall

I think that I made the suggested changes in 86d1773, but I'm not finding any suggested changes for the docstring. Sorry if I'm making some noob mistakes here.

@aucampia

I think that I made the suggested changes in 86d1773, but I'm not finding any suggested changes for the docstring. Sorry if I'm making some noob mistakes here.

See #1710 (comment)

@aucampia

@aucampia

aucampia

@aucampia

Will merge next week, but would be good to get one more approval.

edmondchuc

@nicholascar

I just changed a comment in the code indicating a Datatype that should have been a Class. Also, I've removed geometry. Sorry to be pedantic but if we start adding to a namespace, then we will eventually get in trouble from the standards makers!

Regarding use of a structured literal for geometry like POINT(123.456 -36.78), see GeoSPARQL's hasGeometry: https://github.com/RDFLib/rdflib/blob/master/rdflib/namespace/_GEO.py#L55

aucampia

Choose a reason for hiding this comment

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

nicholascar