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

Single Zenodo record reference #195

Merged
merged 11 commits into from
Nov 2, 2023
Merged

Single Zenodo record reference #195

merged 11 commits into from
Nov 2, 2023

Conversation

jeffjennings
Copy link
Contributor

@jeffjennings jeffjennings commented Nov 1, 2023

Reduces the number of hardcoded Zenodo record refernces in the codebase and docs to one, in __init__.

Maybe changing tests.yml as I have is not ideal for the cache(?), but it's what I could think of to be able to reference anything in the codebase at that point in the workflow.

Closes #194

@jeffjennings jeffjennings mentioned this pull request Nov 2, 2023
3 tasks
Copy link
Collaborator

@iancze iancze left a comment

Choose a reason for hiding this comment

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

Looks good to me. If you're OK with the tests.yml layout then I'd say merge. Thanks!

@@ -13,8 +13,11 @@ jobs:
with:
python-version: 3.8
- name: Install dependencies needed to download files
# we're just installing mpol here to reference the zenodo record number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why the zenodo reference number is needed at this point in the tests? Is it for the later download_external_files.py stage? I think this is a fine solution, but if you really wanted to defer installation of the package until the next stage of the workflow, you could modify the download files to take a path to a zenodo reference, and you would have a script that reads that path from the init.py source file (something similar to the single source Python version but this is probably overkill).

Copy link
Contributor Author

@jeffjennings jeffjennings Nov 2, 2023

Choose a reason for hiding this comment

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

Yes that's right, the reference number is needed for download_external_files.py to reference. It's a little clunky, and your reference is a good one. But if you're ok with a bit of redundancy at this point in the workflow, I'm happy to keep the current workaround.

@jeffjennings
Copy link
Contributor Author

Looks good to me. If you're OK with the tests.yml layout then I'd say merge. Thanks!

Thanks for the quick review!

@jeffjennings jeffjennings merged commit 617275c into main Nov 2, 2023
5 checks passed
@jeffjennings jeffjennings deleted the zenodo_record_ref branch November 2, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Condense Zenodo records references to single instance
2 participants