-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add mapping capabilities #34
Conversation
@csadorf I would say it is now ready for you to review/extend it :) |
I won't push any more to avoid conflicts, but I just thought we might want to make clearer that the mapping name will be used as an id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pablo-de-andres, could you provide examples for the responses of listMappings
and getMapping
?
Based on the SimPARTIX example:
["SimpartixOutput", "SomeOtherSchema"]
{
"name": "SimpartixOutput",
"type": "dlite",
"properties": {
"temperature": "http://emmo.info/emmo#EMMO_affe07e4_e9bc_4852_86c6_69e26182a17f",
"group": "http://emmo.info/emmo#EMMO_0cd58641_824c_4851_907f_f4c3be76630c",
"state_of_matter": "http://emmo.info/emmo#EMMO_b9695e87_8261_412e_83cd_a86459426a28",
}
} |
Some questions:
|
The name is what the response header will use to identify which mapping should be used for the data being sent.
To allow some flexibility/modularity. Could be that the app returns different kinds of data and/or schemas (dlite, cuds, sth else). So you will be told which one applies to the data you received.
I thought this could be useful for the person to know how to interpret the rest of the fields. For example, knowing it is dlite I will know that the mappings apply to the objects inside the
You can always return a list as the value: "temperature": ["IRI1", "IRI2"]
I assume browsing the ontology -> Look for IRI -> read label. But if this is not very user friendly I would say the responsibility lays more in the ontology side (e.g. FOAF has human readable IRIs) |
Thinking about it, the type of the schema (dlite, ...) could also be best sent as part of the response header (with the data), to keep the mapping cleaner. But I think it should be somewhere. |
|
||
@router.get( | ||
"/mappings/{mapping_id}", | ||
operation_id="getMapping", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: getSemanticMapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, endpoint name and operation id should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here: #34 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would potentially need to be changed to /semanticMappings/{semantic_mapping_id}
and getSemanticMapping
.
|
||
@router.get( | ||
"/mappings", | ||
operation_id="listMappings", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: getSemanticMappingIds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at the other capabilities, it seems listSemanticMappings
would be more consistent, and that's also ok for me, although I do think getSemanticMappingIds
is somewhat more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the route and the operation id somewhat consistent so unless we switch this to /semantic-mappings
, I'd recommend to stick to the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. In that case I'd personally go with /semantic-mappings
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor remark: on the global search route we use camelcase (/globalSearch
). I'd encourage following only one style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally prefer to stick to mappings
instead of semanticMappings
, but, agreed, if we decide to change it then the endpoint should be /semanticMappings
and the operationId should be listSemanticMappings
.
@pablo-de-andres Do you want to break the tie?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After an offline discussion with Yoav, I see how the semantic prefix brings some extra information which might be worth the extra characters. I would go with that one.
Summary of the off-line discussion:
Bonus:
|
What is the status of this? Any support needed? |
No, I simply had not had the chance to work on this yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pablo-de-andres I think this is actually more or less done. I just have a few quick questions.
@yoavnash @pablo-de-andres I have commented on the outstanding question regarding the endpoint and operationId names. I don't feel strongly either way so I suggest that the two of you make a decision and then we can move on. |
No description provided.