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

Consider using horned-owl's IRI type instead of strings for entity identifiers #12

Open
balhoff opened this issue Feb 11, 2023 · 5 comments

Comments

@balhoff
Copy link
Member

balhoff commented Feb 11, 2023

All the entity types in whelk have an ID that is represented by a string:

impl Entity {
pub fn id(&self) -> &str {
match self {
Entity::Role(role) => &role.id,
Entity::AtomicConcept(c) => &c.id,
Entity::Individual(i) => &i.id,
}
}
}

In OWL this ID is an IRI. We should think about making this more explicit and directly using IRIs for the IDs. Would this save any memory? (When we translate from OWL to the Whelk model we could pass the IRI directly). Some recent IRI discussion in horned-owl: phillord/horned-owl#56

We should probably wait for any IRI changes in horned-owl to be resolved before taking any action.

@phillord
Copy link

Nice idea. I think it confirms to me that we should not be validating IRIs by default, but only on demand.

It might save some memory but it should also simplify your code a bit -- I think you could ditch all the Rc's that you have currently in model.rs, as IRI already takes care of that (generically). You also gain multi-thread with Arc.

I guess this raises the question of code organisation. For whelk, it makes no difference, but I wonder whether IRI belongs in it's own crate.

@phillord
Copy link

Slightly different question, but I'd quite like to have whelk-rs as a dependency of horned-bin, so I can add a horned-reason command in. Currently that would give us a circular dependency, though. We should have a talk at some point about how to organise this!

@balhoff
Copy link
Member Author

balhoff commented Feb 14, 2023

We're depending on horned-bin so that we can use the parsers (in tests and also in our minimal CLI, which could be a separate product).

@phillord
Copy link

Just the convenience parse_path method as far as I can see. I think there is something to be reworked here, but guess I can leave it will we come to right a horned-reason command.

@balhoff
Copy link
Member Author

balhoff commented Feb 16, 2023

Just the convenience parse_path method as far as I can see. I think there is something to be reworked here, but guess I can leave it will we come to right a horned-reason command.

Oh I see, for our simple CLI we can probably just use the parsers directly if that lets us drop the horned-bin dependency.

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

No branches or pull requests

2 participants