-
Notifications
You must be signed in to change notification settings - Fork 29
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 methods for adding and updating JSON-LD directly (partials for WMS) #149
Conversation
I've added a commit with some changes:
To answer the question in the TODO comment: when updating, any keys starting with Please do add unit tests for the new methods. Better add a new test module for these. |
Thanks for the review @simleo ! Will take a look at your commit and update with tests. |
8821e70
to
9108a90
Compare
c9d09c0
to
2eadd09
Compare
@@ -278,7 +345,7 @@ Note that data entities (e.g., workflows) must already be present in the directo | |||
cd test/test-data/ro-crate-galaxy-sortchangecase | |||
``` | |||
|
|||
This directory is already an ro-crate. Delete the metadata file to get a plain directory tree: | |||
This directory is already an RO-Crate. Delete the metadata file to get a plain directory tree: |
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.
Only place ro-crate
was used, the rest appeared to use RO-Crate
, so I changed the text here to match others.
to, respectively, add a new entity, update an existing entity, or handle | ||
deciding whether to add (if `@id` does not exist in the JSON-LD metadata) | ||
or update an entity automatically. | ||
|
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.
Some example docs, @simleo . Not sure if needed, nor if this is the best place, but given my short memory, I thought it better to write it down somewhere. Let me know if you prefer this to be moved/edited (feel free to edit it too, if you'd like).
test/test_jsonld.py
Outdated
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.
Tests added! Copied the header from other files. Feel free to edit/suggest changes, please 🙇
Thanks!
I've changed the docs to make them more general, since the new methods are also general (as appropriate for a generic RO-Crate library) and useful beyond RO-Crate generation by a WMS. I've also tweaked the code a bit, details are in the commits. Thanks for the contribution! |
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've changed the docs to make them more general, since the new methods are also general (as appropriate for a generic RO-Crate library) and useful beyond RO-Crate generation by a WMS. I've also tweaked the code a bit, details are in the commits. Thanks for the contribution!
Just had a look at the modified docs, and they look great! Thanks a ton for the review & updates, @simleo.
+1
Thanks
Oh, and the test failure happened on macos... in other projects I had to kick macos jobs as they were less reliable then linux on github-actions. Maybe a kick will fix it, otherwise let me know and I can take a look and try to make the build pass. |
I think there's something weird happening with the macOS CI instances. I've added a skipif clause to run that test only on posix. |
Closes #146
@simleo I was toying with the code you suggest a little in the Autosubmit code base. But then I thought that if I wrote those functions in Autosubmit, other WMS would still require to do the same.
It occurred to me, then, maybe add these new methods to
ro-crate-py
instead? I made this draft pull request after testing it with Autosubmit. You can see the current implementation here: https://earth.bsc.es/gitlab/es/autosubmit/-/merge_requests/317/diffsMost of the code in
autosubmit.py
in that merge request is retrieving metadata about the Autosubmit workflow configuration and execution logs and data. But what previously wasNow became, basically
I liked the general structure, and I think it could be applied to other WMS that preferred to go JSON-LD → ro-crate-py → JSON-LD, instead of what I had in the merge request before, YAML → Python → ro-crate-py → JSON-LD (where YAML → Python could possibly vary between WMS's).
If it sounds like a good idea I will add docs & tests 👍 Thank you for the code example.
Also added another commit with comments & f-strings for code that I was having a look, and trying to understand before using it. Let me know if you prefer that I drop that commit and open a separate pull request for it.
Cheers
Bruno