Improving vocabulary module

[filippodebortoli]

Addresses #60 and solves all the issues mentioned there, plus additional refactoring or renaming of some methods. Originally was #61, but having messed up with the git history there, I created a fresh branch and cherry-picked the right commits on top of devel.

It could already be merged. What do you think @althonos @phillord ?

[althonos]

Since your vocab IRIs are all hardcoded somehow, you could implement Borrow<str> for them, and then you could do:

r.build.class(OWL::Thing).into()

which I think from an API perspective is nicer than having to call as_iri_str or as_iri_bytes!

[althonos]

And as another remark I wonder if the WithIRI trait is really necessary, or if you couldn't just instead implement Deref<Target = Iri<String>> for the different namespace types. This way you don't need to implement the as_iri_str or as_iri_bytes yourself.

[filippodebortoli]

Since your vocab IRIs are all hardcoded somehow, you could implement Borrow<str> for them, and then you could do:

r.build.class(OWL::Thing).into()

which I think from an API perspective is nicer than having to call as_iri_str or as_iri_bytes!

For now I implemented Borrow<str> only for the OWL enumeration, but it could be a good idea to do that for the other pieces of terminology eventually.

[filippodebortoli]

And as another remark I wonder if the WithIRI trait is really necessary, or if you couldn't just instead implement Deref<Target = Iri<String>> for the different namespace types. This way you don't need to implement the as_iri_str or as_iri_bytes yourself.

Agreed. I almost entirely could replace the behavior of WithIRI with standard traits such as Deref and TryFrom, there is only one method that is still troublesome which is io::rdf::reader::vocab_lookup where I could not find a way to replace the call to as_iri_str(). Do you have any suggestion?

[althonos]

Agreed. I almost entirely could replace the behavior of WithIRI with standard traits such as Deref and TryFrom, there is only one method that is still troublesome which is io::rdf::reader::vocab_lookup where I could not find a way to replace the call to as_iri_str(). Do you have any suggestion?

The issue is with Deref not giving access to the reference lifetime, so you cannot return a HashMap<&'static str, _> here because the compiler cannot know the reference is static. However, since vocab_lookup is only ever called once when launching a parser, I think vocab_lookup could be made to return a HashMap<String, _> without any actual performance penalty there.

[filippodebortoli]

Agreed. I almost entirely could replace the behavior of WithIRI with standard traits such as Deref and TryFrom, there is only one method that is still troublesome which is io::rdf::reader::vocab_lookup where I could not find a way to replace the call to as_iri_str(). Do you have any suggestion?

The issue is with Deref not giving access to the reference lifetime, so you cannot return a HashMap<&'static str, _> here because the compiler cannot know the reference is static. However, since vocab_lookup is only ever called once when launching a parser, I think vocab_lookup could be made to return a HashMap<String, _> without any actual performance penalty there.

I tried to go for that.
With this, vocab::WithIRI could be deprecated.
The only "issue" is that now we have a lot of duplicated code for each enumeration in vocab.
One solution could be to write a macro that generates the implementations of Deref, TryFrom and Borrow for each such enumeration.

[althonos]

I think you should just delegate to the TryFrom<[u8]> implementation here:

impl TryFrom<&str> for Namespace {
    type Error = HornedError;

    fn try_from(value: &str) -> Result<Self, Self::Error> {
        Self::try_from(value.as_bytes())
    }
}

And also you could implement FromStr if you're gonna implement TryFrom<&str>.

[althonos]

I think this could be all wrapped in a macro that takes care of both the lazy_meta and of the associated trait implementation indeed.

[filippodebortoli]

I think this could be all wrapped in a macro that takes care of both the lazy_meta and of the associated trait implementation indeed.

Done, see vocab::vocabulary_type and vocab::vocabulary_traits.

vocabulary_traits! generates the code implementing FromStr, TryFrom<str>, TryFrom<[u8]> and Deref for each vocabulary type.
vocabulary_type! generates the enumeration associated to a type of vocabulary, produces the IRIs corresponding to each variants by prepending the appropriate namespace and having a flag telling if the name of the variant should be used as-is for the IRI generation or if the first letter should be transformed to lowercase.

[phillord]

I think that this looks good. The complexity of macros added in vocab.rs is more than made up for by the regularity and use of standard traits in the rest of the code.

Do we need to deprecate WithIRI? Or do we just delete it?

[althonos]

You're in 0.* so you could just delete it, that's fine according to semver 👍

[althonos]

By the way I am compiling a list of things to improve regarding Rust conventions (capitalization in some identifiers is not standard, some methods could be renamed to a more standard name like as_x or to_y to convey intent) and traits (e.g. MutableOntology can defer some of the work to the Extend standard trait), I'll create an issue dedicated to that and I'll will likely be very full of API breaks.

[filippodebortoli]

You're in 0.* so you could just delete it, that's fine according to semver 👍

Alright, I updated the code to remove all that was deprecated.
I think the PR is ready for merge after review.

[phillord]

Added one minor review issue. I also wonder about the tests -- they are getting fairly long now, and I wonder whether we should use a "test" module at the end. The reason I worry is that tests tend to churn more rapidly than "real" code and that can make the diffs harder to read. WDYT?

[filippodebortoli]

Added one minor review issue. I also wonder about the tests -- they are getting fairly long now, and I wonder whether we should use a "test" module at the end. The reason I worry is that tests tend to churn more rapidly than "real" code and that can make the diffs harder to read. WDYT?

Addressed in commit de6161e. I could not see the review issue here on GitHub, what is it about?

[filippodebortoli]

If there is no objection or further improvement in this direction I would merge this onto devel.

[phillord]

Good for me