Skip to content
This repository was archived by the owner on Apr 11, 2026. It is now read-only.

provide sophia implementations if the 'sophia' feature is enabled#26

Merged
Tpt merged 4 commits into
oxigraph:masterfrom
pchampin:sophia
Jul 31, 2020
Merged

provide sophia implementations if the 'sophia' feature is enabled#26
Tpt merged 4 commits into
oxigraph:masterfrom
pchampin:sophia

Conversation

@pchampin

Copy link
Copy Markdown
Collaborator

As we discussed previously, here is a PR adding Sophia compliance to Rio.

This is "hidden" behind the optional feature sophia,
so it will not have any impact on Rio users not interested in Sophia.

@Tpt Tpt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for this PR. I just have a global question.
Why integrating all the Sophia adapters in Rio instead of having them in Sophia?
It makes sense for the datamodel where it's trait impls, I have an harder time understanding why it is useful for the parsers where you wrap Rio structs into new ones.

Comment thread api/src/sophia/model.rs
.into()
}
#[inline]
fn datatype(&self) -> Option<SimpleIri<'_>> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dumb question about Sophia. Why not returning rdf:langString for language-tagged strings ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because I'm distracted, and that's what it should do! Thanks for spotting that...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Why integrating all the Sophia adapters in Rio instead of having them in Sophia?

In the end, the sophia crate is meant to become an empty shell re-exporting stuff from other, more specialized crates. And while this is useful for quick prototyping, I would encourage developers of bigger projects to directly pick and chose the specialized package they really need.

Regarding trait impl vs. wrappers: my goal was not (only) to work around Rust limitations for implementing traits on foreign types. It was more generally to add a layer of sophia-compliance atop Rio. Rio provides parsers, it then makes sense that it implements, somehow, the TripleParser/QuadParser traits of Sophia. Since Rio and Sophia have slightly different notions of what a parser is, this needs a few wrappers, but these wrappers are slim enough (I think).

Of course, if you disagree, I will just leave the trait impl and move the wrappers to a separate crate in the Sophia repo.

@Tpt

Tpt commented Jul 18, 2020

Copy link
Copy Markdown
Collaborator

To say the truth I would feel better if you keep the Sophia wrappers in a separate crate for now, if it ok for you. We could add to the Rio README a link to it. Sorry for having wasted a bit of your time with this pull request.

@pchampin

Copy link
Copy Markdown
Collaborator Author

To say the truth I would feel better if you keep the Sophia wrappers in a separate crate for now, if it ok for you. We could add to the Rio README a link to it. Sorry for having wasted a bit of your time with this pull request.

No problem, I understand perfectly :-) I'll remove the *Parser wrappers from this PR.

Maybe we could meet halfway though, and keep StrictRioSource and GeneralizedRioSource in rio_api?? Otherwise, I would need to put this is yet another crate, as it will be shared by sophia_turtle and sophia_xml...
If really you don't want any wrapper, I could refactor the code, and have Sophia's TripleSource and QuadSource implemented directly by Result<rio_api::TriplesParser, _> and Result<rio_api::QuadsParser, _>.

@Tpt

Tpt commented Jul 18, 2020

Copy link
Copy Markdown
Collaborator

I could refactor the code, and have Sophia's TripleSource and QuadSource implemented directly by Result<rio_api::TriplesParser, _> and Result<rio_api::QuadsParser, _>.

Would #27 solve this problem? It makes all the new methods return a TripleParser or a QuadParser instead of a Result. But it adds oxiri::Iri to the public API.

@pchampin

Copy link
Copy Markdown
Collaborator Author

Would #27 solve this problem?

I don't mind either way. Anyway, I think I will need to wrap it inside a Result because building the TriplesParser can still fail (if not in new itself, it wille fail during the construction of the IRI required by new), and I want that case yield a TripleSource as well...

@Tpt

Tpt commented Jul 18, 2020

Copy link
Copy Markdown
Collaborator

I see. I would feel better if everything related to the Sophia parsers are in the sophia_turtle and sophia_xml crates. sophia_xml only provides a TripleParser so you might just do a small code duplication instead of creating a sophia_rio_api crate.

@Tpt

Tpt commented Jul 29, 2020

Copy link
Copy Markdown
Collaborator

Do you want to move forward with this PR? I would be very happy to add the implementation of Sophia traits to RioApi datamodel. I could do it if you prefer. I plan to do a new release of Rio soon before releasing Oxigraph 0.1

instead, introduce macros in rio_api to automate the creations of adapters:
- Rio's TriplesParser -> Sophia's TripleSource
- Rio's QuadsParser -> Sophia's QuadSource
- Rio's GeneralizedQuadsParser -> Sopia's QuadSource
@pchampin

Copy link
Copy Markdown
Collaborator Author

Sorry about the delay. I just pushed a new version, where I removed all the Sophia-specific types implementing Sophia's parser traits.

Instead, I simply implement TripleSource / QuadSource (which is a straightforward match) for Rio's parsers.

The only Sophia-specifc items that are still exposed by rio_api are:

  • the macros I used to factorize code for the trait implementations, and
  • RioStreamError, a zero-cost wrapper around Sophia's StreamError which is used in all macros.

If really you don't want to expose anything (beyond trait impl), I can move them into rio_turtle and duplicate what needs to be duplicated in rio_xml.

@Tpt

Tpt commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator

Thank you!

If really you don't want to expose anything (beyond trait impl), I can move them into rio_turtle and duplicate what needs to be duplicated in rio_xml.

Yes, I would feel better to not expose these implementation details in rio_api to make this crate only focus on the public API.

now the 'sophia' features only enables trait impl;
it adds nothing to the exposed API of the crates.
@pchampin

Copy link
Copy Markdown
Collaborator Author

Yes, I would feel better to not expose these implementation details in rio_api to make this crate only focus on the public API.

Done

@Tpt Tpt merged commit eb81286 into oxigraph:master Jul 31, 2020
@Tpt

Tpt commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants