Conversation
…h DatabaseSource.
…l query. Ongoing work.
…ill missing to define the search method.
…ed. Missing mappings.
…, sparql, and tests.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #339 +/- ##
==========================================
- Coverage 74.30% 72.94% -1.36%
==========================================
Files 93 104 +11
Lines 5934 6635 +701
==========================================
+ Hits 4409 4840 +431
- Misses 1525 1795 +270
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
kgforge/core/archetypes/store.py
Outdated
| return f"{pfx}\n{qr}" | ||
|
|
||
|
|
||
| def resources_from_construct_query(data, context): |
There was a problem hiding this comment.
fun, we have the same one!
There was a problem hiding this comment.
ha! I guess it was a "clear need"
There was a problem hiding this comment.
for this and the next, what would be the cleanest solution? I would like to use your code as it looks more organized, and I agree it fits well in the SPARQLQueryBuilder
There was a problem hiding this comment.
Cleanest solution in what sense?
(I've put the resource building methods in SparqlQueryBuilder following @MFSY 's comment, but I am of the opinion that there should be another Class for sparql response parsing, as it has nothing to do with query building. I guess the query builder can be treated as a set of helper functions for sparql, in which case maybe renaming it would be better)
There was a problem hiding this comment.
Yes, the latter is what it's happening. I would not mind also to have just a class for parsing.
The question is ... should I keep things as I have, and whenever your branch is implemented I make another PR to use it?
There was a problem hiding this comment.
Yes I'm not too sure about that, that's what I had meant on slack that i'd be reluctant to start an implementation for the read only store because it can benefit from work in our 2 branches
There was a problem hiding this comment.
I think it's a matter of which is merged first. But I don't think you should try to make the changes on this branch fit the ones on mine, that will happen when any of the branches needs to rebase to master
crisely09
changed the title
|
Having the forge object in the database feels off |
| Example: NeuroMorpho uses response["_embedded"]["neuronResources"] | ||
| which should be given as: response_loc = ["_embedded", "neuronResources"] | ||
| """ | ||
| response_location = params.pop('response_loc', None) |
There was a problem hiding this comment.
I think it would be clearer to have params as exactly the params being passed to requests.get, and response_loc to be a separate parameter of resources_from_request. You can do the popping if it's necessary before the resources_from_request call
There was a problem hiding this comment.
And params can be not keyword arguments but a dict
kgforge/core/archetypes/database.py
Outdated
| mapped_resources.append(self._forge.map(resource_dict, type_)) | ||
| elif type_ in mappings: | ||
| # type_ is the entity here | ||
| mapping_class : Mapping = import_class(mappings[type_][0], "mappings") |
There was a problem hiding this comment.
The mapping_class is of type Type[Mapping] not Mapping
| def __repr__(self) -> str: | ||
| return repr_class(self) | ||
|
|
||
| def map(self, resources : Union[List[Union[Resource, str]], Union[Resource, str]], |
There was a problem hiding this comment.
- If I understand correctly, if type_ is not provided, a resource is mapped according to the mapping that is associated with its (the resource’s) type property by model.
- The if else on the resource type assumes that if it’s not of type Resource, it’s of type Dict, but the typing in map defines it to be a Union (or list of) of Resource and str. Should it be dict instead?
- If a resource has no type_ and no type is provided, there is a silent error?
- Similarly there is a series of ifs related to type_ and if neither of the cases that are “valid” are true then it’s silently considering the resource as mapped. Shouldn’t there be some warning? Similar to with the update/register operations, it gives the count of successful operations and failed ones and error messages linked to the failures.
- (In the last two points I’ve talked about “errors” but really it’s just considering a resource to be mapped when it’s not been. However, the thing that would happen is at least that if the element provided was a dictionary it would be converted into a Resource)
| return self._service_from_web_service(source, **source_config) | ||
| elif origin == "store": | ||
| store = import_class(source, "stores") | ||
| if source != 'DemoStore': |
There was a problem hiding this comment.
Why do you need this check?
| return self._service_from_web_service(source, **source_config) | ||
| elif origin == "store": | ||
| store = import_class(source, "stores") | ||
| if source != 'DemoStore': |
There was a problem hiding this comment.
Why do you need this check?
| from kgforge.specializations.mappings import DictionaryMapping | ||
| from kgforge.specializations.databases import StoreDatabase, WebServiceDatabase | ||
|
|
||
|
|
There was a problem hiding this comment.
you should declare _db_sources as a property, even if it's not defined on initialisation. So that it's clear all attributes the forge object may have
There was a problem hiding this comment.
Or should create_db_sources be called on forge initialisation? It seems like it isn't happening so forge doesn't have db sources
| """Expose the context used in the model.""" | ||
| return self._model.context() | ||
|
|
||
| def create_db_sources(self, all_config: Optional[Dict[str, Dict[str, str]]], |
There was a problem hiding this comment.
This doesn't seem to be called anywhere?
4 participants
There is an error in codecov in PR #360, it may be related to the fact that the branch is located in a forked repo.