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.

Sign up for GitHub

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

Jump to bottom

Introduce WGS84 DefinedNamespace #1710

Merged
merged 6 commits into from Feb 21, 2022
Merged

Introduce WGS84 DefinedNamespace #1710

merged 6 commits into from Feb 21, 2022

Conversation

kvjrhall
Copy link
Contributor

@kvjrhall kvjrhall commented Feb 9, 2022

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

  • Introduce _WGSS84 namespace
  • Export the namespace as rdflib.namespace.WGS84
  • Add wgs84 as a default prefix for the namespace

@kvjrhall
Introduce WGS84 DefinedNamespace
c1b3aa8 
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
Change prefix wgs84 to wgspos
e31e515 
The more commonly used prefix for wgs84 is wgspos, at least when it's
[looked up](https://prefix.cc/wgspos) through sites like prefix.cc. It makes
sense to use that rather than wgs84 which likely hasn't seen any use.
@kvjrhall
Copy link
Contributor Author

kvjrhall commented Feb 9, 2022

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
Copy link
Contributor

For what it's worth, I've seen the prefixes wgs and wgs84 used but not wgspos. I'd vote for wgs simply because it's shorter.

See:

@kvjrhall
Assign WGS84 the 'wgs' prefix and not 'wgspos'
449232e 
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
Rename class WGS84 to WGS to match prefix name
86d1773
@kvjrhall
Copy link
Contributor Author

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

aucampia
aucampia reviewed Feb 10, 2022
View reviewed changes
rdflib/namespace/__init__.py Outdated
@@ -75,6 +75,7 @@
* TIME
* VANN
* VOID
* WGS84
Copy link
Member

@aucampia aucampia Feb 10, 2022 •
edited

Choose a reason for hiding this comment

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

Suggested change
* WGS84
* WGS

aucampia
aucampia requested changes Feb 10, 2022
View reviewed changes
Copy link
Member

@aucampia aucampia left a comment

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
kvjrhall commented Feb 11, 2022
View reviewed changes
Copy link
Contributor Author

@kvjrhall kvjrhall left a comment •
edited

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

Fixed it now in 0208526

@aucampia aucampia added the review wanted This indicates that the PR is ready for review label Feb 19, 2022
@aucampia
Copy link
Member

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

@nicholascar
Copy link
Member

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
aucampia approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

Rechecked after @nicholascar 's changes

@nicholascar nicholascar merged commit 90e804f into RDFLib:master Feb 21, 2022
@kvjrhall kvjrhall mentioned this pull request Mar 8, 2022
Closed
@aucampia aucampia mentioned this pull request Mar 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aucampia aucampia aucampia approved these changes

@nicholascar nicholascar nicholascar approved these changes

@edmondchuc edmondchuc edmondchuc approved these changes

@ashleysommer ashleysommer Awaiting requested review from ashleysommer

@white-gecko white-gecko Awaiting requested review from white-gecko

Assignees
No one assigned
Labels
review wanted This indicates that the PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Intoduce WGS-84 DefinedNamespace
4 participants
@kvjrhall @edmondchuc @aucampia @nicholascar