Repository navigation
feat(onto creation): allow that resources and properties are not sorted by inheritance (DEV-626) - #167
Conversation
|
Should the "not" in the title of the PR be removed? |
|
No, it's correct. During onto creation, the res & props need not be sorted by inheritance. Do you think there is a better title with less misunderstandings? |
irinaschubert
left a comment
irinaschubert
left a comment
There was a problem hiding this comment.
In order to know that the two methods are doing what you expect, it would be good to add unit tests for them.
|
|
||
| def sort_resources(ontology: dict[Any, Any]) -> list[dict[Any, Any]]: | ||
| """ | ||
| In case of inheritance, parent resource classes must be uploaded before their children. This method sorts the |
There was a problem hiding this comment.
In my opinion, docstrings should just describe what the method / class does. Mentioning / explaining the upload is out of scope here. I would keep it as concise as possible like "This method sorts the resource classes in an ontology according their inheritance order (parent classes first)."
| ontology: an ontology definition from a JSON file | ||
|
|
||
| Returns: | ||
| the sorted list of resource classes, in the format as it was in the JSON file |
There was a problem hiding this comment.
| the sorted list of resource classes, in the format as it was in the JSON file | |
| the sorted list of resource classes |
| resource classes accordingly. | ||
|
|
||
| Args: | ||
| ontology: an ontology definition from a JSON file |
There was a problem hiding this comment.
| ontology: an ontology definition from a JSON file | |
| ontology: the ontology definition |
| unsorted_resources: list[dict[Any, Any]] = ontology['resources'] | ||
| sorted_resources: list[dict[Any, Any]] = list() | ||
| ok_resource_names: list[str] = list() | ||
| while len(unsorted_resources) > 0: |
There was a problem hiding this comment.
Do you need this when afterwards there is a for loop?
There was a problem hiding this comment.
Yes. The for-loop needs to repeat until there are no more unsorted resources. One for-loop alone cannot do it.
| if isinstance(parent_classes, str): | ||
| parent_classes = [parent_classes] | ||
| parent_classes = [re.sub(r'^:([^:]+)$', f'{onto_name}:\\1', elem) for elem in parent_classes] | ||
| parent_classes_okay = [not parent.startswith(onto_name) or parent in ok_resource_names for parent in parent_classes] |
There was a problem hiding this comment.
| parent_classes_okay = [not parent.startswith(onto_name) or parent in ok_resource_names for parent in parent_classes] | |
| parent_classes_ok = [not parent.startswith(onto_name) or parent in ok_resource_names for parent in parent_classes] |
| parent_classes = [parent_classes] | ||
| parent_classes = [re.sub(r'^:([^:]+)$', f'{onto_name}:\\1', elem) for elem in parent_classes] | ||
| parent_classes_okay = [not parent.startswith(onto_name) or parent in ok_resource_names for parent in parent_classes] | ||
| if all(parent_classes_okay): |
There was a problem hiding this comment.
| if all(parent_classes_okay): | |
| if all(parent_classes_ok): |
| sorted_resources: list[dict[Any, Any]] = list() | ||
| ok_resource_names: list[str] = list() | ||
| while len(unsorted_resources) > 0: | ||
| for res in unsorted_resources.copy(): |
There was a problem hiding this comment.
Because inside the for-loop, I remove items from the original list. As far as I know, you shouldn't edit a data structure you are iterating over during the iteration process.
| if all(parent_classes_okay): | ||
| sorted_resources.append(res) | ||
| ok_resource_names.append(res_name) | ||
| unsorted_resources.remove(res) |
There was a problem hiding this comment.
I'm not sure, but can't you omit the while clause and this line and just have the for loop?
There was a problem hiding this comment.
If only one parent class is not okay, I cannot declare a resource as okay. In such a case, the for-loop will continue, and in one of the later passes, the not-okay parent will become okay.
|
|
||
| def sort_prop_classes(ontology: dict[Any, Any]) -> list[dict[Any, Any]]: | ||
| """ | ||
| In case of inheritance, parent properties must be uploaded before their children. This method sorts the |
There was a problem hiding this comment.
Same as above, I would try to be as concise as possible with docstrings.
Also, all the above comments can be applied to this method, too.
All good, I thought the PR is about something else. But now I got it. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Hi @irinaschubert, thanks for your comments. I responded to them and added unit tests. Can you re-review it? |
2 participants
resolves DEV-626