[merged] Introduce `map()` and helpers to `Term` and similars

pchampin / sophia_rs

Sophia: a Rust toolkit for RDF and Linked Data

Other

228 stars 24 forks source link

[merged] Introduce `map()` and helpers to `Term` and similars #61

Closed MattesWhite closed 4 years ago

MattesWhite commented 4 years ago

Allows better control over ownership and transformation of 'TermData'.

The idea originated from this recent topic at URLO: https://users.rust-lang.org/t/is-it-possible-to-impl-from-wrapper-t-for-wrapper-u/40029/3

With map() is is possible to directly map owned TermData. At the moment transformation is usually done through reference via Self<T>: From<&Self<U>>. This rather implicit conversion can now be replaced by more explicit self.as_ref().map(...). Furthermore, this can become handy for handling intermediate Strings (#60). Another example is term.as_ref_str() instead of RefTerm::from(&term).

WIP

At the moment this PR is more a PoC and methods are only implemented for Iri.

pchampin commented 4 years ago

I like the idea of map very much.

A few remarks

I would allow the function passed to map to alter the underlying text. Doing otherwise would break the user's expectation about what map does (in any other type, this is the main purpose of map after all). Granted, modifying the string is more hazardous here, since the function does not know the context of the string (is it a ns? a suffix? the value of a literal?...). But I still think this sould be allowed.
I think we could then simply get rid of clone_with, which essentially does the same thing -- or maybe rename it to map_str, for the sake of regularity.

pchampin commented 4 years ago

Just an afterthought: I'd like to keep clone_with after all, and renamingir map_str would not be a good idea: map (and map_X) consumes the term, while clone (and hence clone_X) only borrows it.

MattesWhite commented 4 years ago

With the latest commit this PR is now ready to be merged from my perspective.

BTW the replacement of .into() by .clone_into() revealed many opportunities to optimize implementations, I think.

pchampin commented 4 years ago

@MattesWhite please do not merge master in the middle of a PR. I'd rather you rebase it on top of master and force push it -- or do it myself just before the merge. It makes the history much nicer (call this vanity :wink:).

Apart from that, very nice work, thanks. I'll do the tidying and merge it.