Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[#72] added service for single entity #73

Merged
merged 3 commits into from Mar 25, 2017

Conversation

Projects
None yet
3 participants
Collaborator

jnguyenx commented Mar 16, 2017

I'm not sure about the expected output, can you please have a look at the unit test?

@jnguyenx jnguyenx requested a review from cmungall Mar 16, 2017

+ throws UnknownFilterException, IncoherentStateException {
+
+ // TODO it's weird to resolve the curie here
+ String resolveEntity = curieUtil.getIri(entity).orElse(entity);
@julesjacobsen

julesjacobsen Mar 17, 2017

Contributor

This is a common pattern using the CurieUtil - might be worth providing a toIriString() method?

+ required = true) @PathParam("ontology") String ontology)
+ throws UnknownFilterException, IncoherentStateException {
+
+ // TODO it's weird to resolve the curie here
@julesjacobsen

julesjacobsen Mar 17, 2017

Contributor

But you're passing the entity into the ClassMatcher via the matchEntity() method so you have the curieUtil accessible from you in there and don't need to do it here.

@jnguyenx

jnguyenx Mar 17, 2017

Collaborator

You mean passing the CurieUtil as a parameter of matchEntity()?

I'm wondering if the ClassMatcher should have a CurieUtil in the constructor, what do you think?

@julesjacobsen

julesjacobsen Mar 20, 2017 edited

Contributor

Ahh, no sorry - I'm mistaken. The curieUtil is used internally within the BMKnowledgebase, but is not accessible from it. Providing the ClassMatcher with the curieUtil and bmKnowledgeBase in the constructor would solve this. Then you can move

String resolveEntity = curieUtil.getIri(entity).orElse(entity); 

into the ClassMatcher.

I just wonder to what extent we need to hide the CurieUtil inside the BMKnowledgeBase - presumably we're always going to use the OWLAPI and the CurieUtil in the BMKnowledgeBase? If that's the case and these are going to be required externally to query the BMKnowledgeBase, then it should expose them so they can be used, or provide methods like:

public String resolveIri(String entity)
    return curieUtil.getIri(entity).orElse(entity); 
@jnguyenx

jnguyenx Mar 20, 2017

Collaborator

Yep I was also leaning towards that solution, I'll make that change in my PR!

Thanks!

@julesjacobsen

julesjacobsen Mar 21, 2017 edited

Contributor

I meant to add the resolveIri() method to the BMKnowledgeBase interface, in case I wasn't clear - which I wasn't!

Owner

cmungall commented Mar 20, 2017

My intent was to use the curieUtil when constructing the KB, and to use curies internally, obviating the need for multiple classes to know about it. Exposing via services is a good idea, for clients that speak URIs

Collaborator

jnguyenx commented Mar 21, 2017

Okay modified!

Collaborator

jnguyenx commented Mar 23, 2017

@julesjacobsen @cmungall when you have time, can you have a look?

@jnguyenx jnguyenx merged commit c4afa28 into master Mar 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jnguyenx jnguyenx deleted the 72-ontomatch-single-entity branch Mar 25, 2017

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