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 fragment property to URIRef #1991

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Conversation

ottokruse
Copy link
Contributor

Summary of changes

Added a property fragment to URIRef so it is easy to access the URI's fragment:

>>> URIRef("http://example.com/some/path/#some-fragment").fragment
'some-fragment'
>>> URIRef("http://example.com/some/path/").fragment
''

Why is this useful?

The non-fragment part of the URI often just serves a namespace purpose, e.g. https://brickschema.org/schema/Brick#.

Actual "things" use that namespace, and have the "thing" name as the fragment, e.g. https://brickschema.org/schema/Brick#Valve_Position_Sensor

For display purposes, it's nice to get to the "thing" name, i.e. the fragment easily.

TODO: update docs and tests. I'll do that if you deem this PR is useful.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@nicholascar nicholascar requested review from nicholascar, aucampia and a user June 15, 2022 09:04
Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

I'm ok with this without tests as it's just passing on an existing function from urlparse().

I try to never use hash URIs with fragments but if you do use them, I can see this might be nice.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

yup, straightforward enough to merge.

@aucampia
Copy link
Member

pre-commit.ci autofix

@coveralls
Copy link

coveralls commented Jun 15, 2022

Coverage Status

Coverage increased (+0.002%) to 88.557% when pulling b87a11d on ottokruse:uriref-fragment into d263009 on RDFLib:master.

@ghost
Copy link

ghost commented Jun 16, 2022

yup, straightforward enough to merge.

aaaand ... nice work, btw 😄

@ottokruse
Copy link
Contributor Author

Awesome! And now, when will this be merged? And released to pip?

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.

While this is useful, it is also a bit arbitrary, as we don't have scheme, netloc, path, params or query. But I think on balance it does not really do any harm to include it, and we can just deprecate it if it does cause problems down the line.

@aucampia aucampia merged commit a6e3da4 into RDFLib:master Jun 17, 2022
@aucampia
Copy link
Member

@ottokruse thanks for the PR, we plan to make the next release around the middle of July.

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.

4 participants