-
Notifications
You must be signed in to change notification settings - Fork 0
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
changes to add new version to exisitng deposition #4
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks Akshat, it looks good overall, but I left a few suggestions. Can we also change the PR title to cover all the changes: disable old tests, refactor XYZ, add new test etc?
src/zenodopy/zenodopy.py
Outdated
@@ -351,7 +362,9 @@ def list_files(self): | |||
# except UserWarning: | |||
# warnings.warn("The object is not pointing to a project. Either create a project or explicity set the project'", UserWarning) | |||
|
|||
def create_project(self, title=None, upload_type=None, description=None): | |||
def create_project( | |||
self, title=None, upload_type=None, metadata_json=None, description=None |
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.
upload_type
is unused?
zeno.bucket | ||
zeno.deposition_id | ||
zeno.sandbox | ||
# def test_get_key(): |
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.
Would be good to put a comment above each disabled test saying why it no longer works and when it can be brought back
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.
added a note at the start of the file
""" | ||
This module contains tests for the zenodopy library using pytest. | ||
Functions: | ||
test_client: Tests the initialization of the zen.Client object with and without a token. | ||
test_read_config: Tests the _read_config method of the zen.Client object to ensure it raises a TypeError. | ||
test_get_baseurl: Tests the _endpoint attribute of the zen.Client object for both sandbox and production environments. | ||
test_get_depositions: Tests the _get_depositions, _get_depositions_by_id, and _get_depositions_files methods of the zen.Client object. | ||
test_get_bucket: Tests the _get_bucket_by_id method of the zen.Client object. | ||
test_get_projects_and_files: Tests the list_projects and list_files properties of the zen.Client object. | ||
Note: | ||
The update and change_metadata functions have been updated to add new versions to existing depositions. | ||
This functionality is being tested in test_version. We will bring back individual tests once these changes | ||
have been merged upstream to keep the changes incremental. | ||
""" |
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.
@siddharth-krishna
does this comment convey the plan clearly ?
I can update it to be more clear
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.
It does, but I still feel each individual line is better placed as a docstring/comment for the appropriate test function. Also, that way someone who jumps to the middle of the file will see why something is disabled, or what a function is testing. But this isn't critical, can be done later too.
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.
Thanks Akshat, it looks good overall. A few suggestions:
- When you make the upstream PR, include in the description things like: why we disabled some tests, why we deleted
coverage.yaml
, why we disabled tests for older python versions, why replaced tox with pytest, and of course why we had to change the way version updates work.
""" | ||
This module contains tests for the zenodopy library using pytest. | ||
Functions: | ||
test_client: Tests the initialization of the zen.Client object with and without a token. | ||
test_read_config: Tests the _read_config method of the zen.Client object to ensure it raises a TypeError. | ||
test_get_baseurl: Tests the _endpoint attribute of the zen.Client object for both sandbox and production environments. | ||
test_get_depositions: Tests the _get_depositions, _get_depositions_by_id, and _get_depositions_files methods of the zen.Client object. | ||
test_get_bucket: Tests the _get_bucket_by_id method of the zen.Client object. | ||
test_get_projects_and_files: Tests the list_projects and list_files properties of the zen.Client object. | ||
Note: | ||
The update and change_metadata functions have been updated to add new versions to existing depositions. | ||
This functionality is being tested in test_version. We will bring back individual tests once these changes | ||
have been merged upstream to keep the changes incremental. | ||
""" |
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.
It does, but I still feel each individual line is better placed as a docstring/comment for the appropriate test function. Also, that way someone who jumps to the middle of the file will see why something is disabled, or what a function is testing. But this isn't critical, can be done later too.
I will bring back individual tests in next PR and add coverage and other things it causes CI to fail so only testing with pytest, |
remove title from create_project
set_project
can now also be update parent DOI113600
ID for106299
new ZenodoMetaData
dataclass
updated
change_metadata
function using new ZenodoMetadata classZenodoMetadata class can be extended to include these properties Zenodo docs metadata
update
function addspublication_date
to metadata if empty which is required to publish a new versionall internal functions are being tested
version update is being tested in
test_vesion.py
using CIindividual tests for
create_project, set_project, change_metadata
are still missing even tough they are being tested together in CI