(Draft) New Data Model#480
Conversation
This reverts commit 4fe4ffb.
This reverts commit df5b0ef.
This reverts commit d825217.
This reverts commit 8b4f9a3.
Pull in relevant changes from `develop`
Disable tests that fail due to data model
Resolve #383: Test `ld_context`
…or/381-test-ld_container # Conflicts: # poetry.lock
Update poetry.lock for added test dependencies.
Refactor/381 test ld container
Fix handling of identifier in compaction and expansion.
Fix #454: Add end-to-end tests for the plugin-related `SoftwareMetadata` API
…c-api Refactor/423 implement public api
zyzzyxdonta
left a comment
There was a problem hiding this comment.
Some first very high-level comments. Will continue the review another day.
There was a problem hiding this comment.
Should a top-level conftest exist? I think this belongs into test/ or test/hermes_test/
| for mapping in active_ctx["mappings"].values(): | ||
| if "@container" in mapping and long_iri: | ||
| value = {x: "none" for x in mapping["@container"]} |
There was a problem hiding this comment.
I don't understand what this is for.
The and long_iri I guess saves work if no IRI is given. But this could be expressed more clearly by doing if long_iri is None: return long_iri at the beginning of the function, just like _compact_iri() does it.
value overwritten in each new iteration in each iteration because it @container can only exist once? In that case, this would be clearer by breaking out of the loop.
But what does "none" do?
I think this whole module goes deep into pyld internals and a couple of comments would be helpful to understand what is going on.
There was a problem hiding this comment.
I only remember that there was some bug with the compaction. This fix was written by @SKernchen, who might be able to answer your questions.
As for the function of "none": I believe it tells the JSON-LD processor that a value should be dropped.
There was a problem hiding this comment.
With the given poetry.lock, the docs don't compile. A lot of our packages are old and incompatible with the ones that aren't pinned. I got it to work like this:
diff --git i/pyproject.toml w/pyproject.toml
index 17ea508..932427b 100644
--- i/pyproject.toml
+++ w/pyproject.toml
@@ -92,13 +92,13 @@ pytest-httpserver = "^1.1.5"
optional = true
[tool.poetry.group.docs.dependencies]
-Sphinx = "^6.2.1"
+Sphinx = "^8.0.0"
# Sphinx - Additional modules
-myst-parser = "^2.0.0"
+myst-parser = "^4.0.0"
sphinx-book-theme = "^1.0.1"
sphinx-favicon = "^0.2"
sphinxcontrib-contentui = "^0.2.5"
-sphinxcontrib-images = "^0.9.4"
+sphinxcontrib-images = "^1.0.1"
sphinx-icon = "^0.1.2"
sphinx-autobuild = "^2021.3.14"
sphinx-autoapi = "^3.0.0"There was a problem hiding this comment.
I had to remove 'sphinxemoji' too or constrain setuptools to <82 (the dependency pkg_resources (deprecated) was removed in setuptools version 82 but was necessary for 'sphinxemoji').
Also building the docs loally always (at least for me) results in errors from myst (failed linkings).
This has been an issue for longer though, but I haven't been able to fix it so far.
There was a problem hiding this comment.
Yup, the setuptools change was exactly the same problem I had. I didn't have any problems with sphinxemoji though. Strange ... 🤔
Yes, the errors from myst have been there for a while. I think it's mostly unused or missing references/labels. I haven't looked into it.
There was a problem hiding this comment.
Removing 'sphinxemoji' caused no errors or warnings. Therefore leaving it removed should be fine.
I'll look into it when I find time.
| it will always be returned in a **list**-like object! | ||
| ``` | ||
|
|
||
| The reason for providing data in list-like objects is that JSON-LD treats all property values as arrays. |
There was a problem hiding this comment.
Link to the related section in the JSON-LD spec would be nice
| Python data: | ||
|
|
||
| ```{code-block} python | ||
| :caption: Naive containment assertion that raises |
There was a problem hiding this comment.
It doesn't though. The mock output is from the path where the assertion holds.
There was a problem hiding this comment.
I'll look through the entire file and correct all other outdated information.
| } | ||
| ``` | ||
|
|
||
| HERMES would use the {py:class}`~hermes.model.merge.action.Reject` strategy for merging values of the key `full_property_iri` in objects of type `full_type_iri`. (A key in strategies being `None` instead of a string indicates to HERMES that its value is to be used as a default [i.e. if no more specific entry exists].) |
There was a problem hiding this comment.
This feels hacky. Have you considered collections.defaultdict?
There was a problem hiding this comment.
No, I hadn't.
But now that I looked into it, I don't see the benefit.
Sure the None keys would be no longer necessary, which means in one spot no checking for none existing keys. Instead we need to handle the default_factory while merging the strategies.
(And I find it more intuitive with None.)
There was a problem hiding this comment.
This whole file feels more like a reference than a tutorial. A tutorial should be "learning by doing". An example could be having the reader re-build the git-plugin from scratch.
See also: https://diataxis.fr/tutorials/
There was a problem hiding this comment.
I moved it into the devs section and will add real tutorials, but that may take some time.
Should there be one tutorial for each steps plugin, or could one be enough?
This pull request is not ready yet. It is only for showing the process