Skip to content
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

Publish links #390

Open
wants to merge 5 commits into
base: main-bak
Choose a base branch
from
Open

Publish links #390

wants to merge 5 commits into from

Conversation

blue442
Copy link
Collaborator

@blue442 blue442 commented Aug 1, 2023

Addressing #187 by adding the functionality where foundry.publish_dataset() can take links in the form of a keyword argument. The arguments associated with the links keyword are of the following form:
{"type":str, "doi":str, "url":str, "description":str, "bibtex":str}
following the convention laid out in the MDF data schemas, and can be included as a single argument or a list of arguments.

Some light validation is performed to ensure that the keys in each of the link conform to the options described in the MDF schema (but are all optional); violation results in a ValueError being thrown.

After validation the links are passed to the add_links() function from the MDF connect client.

Tests were included in test_foundry.py to test the successful addition of links as well as ensuring the validation process works successfully.

NOTE: This branch was based off of the reviving_load branch which is yet to be merged to main, and therefore appears to contain more changes than intended.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #390 (fc12b1f) into main (0ccb39b) will decrease coverage by 1.39%.
Report is 2 commits behind head on main.
The diff coverage is 8.33%.

❗ Current head fc12b1f differs from pull request most recent head b57c1cc. Consider uploading reports for the commit b57c1cc to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
- Coverage   72.08%   70.70%   -1.39%     
==========================================
  Files           9        9              
  Lines         541      553      +12     
==========================================
+ Hits          390      391       +1     
- Misses        151      162      +11     
Files Changed Coverage Δ
foundry/foundry.py 57.41% <8.33%> (-1.98%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ascourtas ascourtas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! All that's left to finalize this is updates in the GitBook documentation and in the example notebooks (under /examples). Let me know if you're having trouble with the GitBook documentation -- you can open an Edit Request which is like a PR there

@ascourtas
Copy link
Contributor

ascourtas commented Aug 7, 2023

the publishing example addition looks mostly very good, thank you steve! could you just add what the other options are in the comment above the example links? And in the documentation, it would be good to define the different options (eg "other_paper", etc) more thoroughly.

Also, can you clear the output of this cell and repush the publish example notebook?
Screen Shot 2023-08-07 at 9 44 43 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants