Skip to content

[RFC] Add initial invenio class hierarchy #451

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

Closed
wants to merge 1 commit into from

Conversation

oerc0122
Copy link
Collaborator

Add class hierarchy for handling pushing to invenio-like data repositories.

@oerc0122 oerc0122 added the enhancement New/improved feature or request label Feb 26, 2025
@oerc0122 oerc0122 self-assigned this Feb 26, 2025
@oerc0122 oerc0122 force-pushed the add-invenio-repo-class branch 2 times, most recently from cd2cfce to fa03559 Compare February 26, 2025 17:33
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Generally looks good in principle, thanks @oerc0122.

Most of the comments relate to minor things that the various checks probably complain about anyway, although they're not exhaustive.

What's the best way to test this? Mocking?

self.name = name

@property
def dep_id(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def dep_id(self):
def dep_id(self) -> str:

return self.parent.dep_id

@property
def bucket_url(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def bucket_url(self):
def bucket_url(self) -> str:

my_repo.liceses.list()
"""

def __init__(self, url: str, api_key: str):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, url: str, api_key: str):
def __init__(self, url: str, api_key: str) -> None:

--------
.. code-block::

my_repo = InvenioRepository("abc123", "companyname.website")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
my_repo = InvenioRepository("abc123", "companyname.website")
my_repo = InvenioRepository("companyname.website", "abc123")

Parameters
----------
dest
Folder to write files to.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Folder to write files to.
Folder to write files to. Default is current working directory.

requests.Request
Status of operation.
"""
file = Path(file)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check if the path is actually a file, rather than a directory?

"""
request_list = []
for name, file in files.items():
file = Path(file)
Copy link
Member

Choose a reason for hiding this comment

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

What if the path is a directory?

(Do we want to be able upload all files in a directory?)

class _Deposition(_SubCommandHandler):
"""Deposition handler."""

def __init__(self, parent, dep_id):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, parent, dep_id):
def __init__(self, parent, dep_id) -> None:

import requests


def _check(request: requests.Request, proc: str):
Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring and return type hint

Comment on lines +22 to +23
class _SubCommandHandler(ABC):
def __init__(self, parent):
Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring/type hint

my_repo = InvenioRepository("abc123", "companyname.website")
my_repo.depositions["my_repo"].files["file"].upload(my_file)
my_repo.records.get()
my_repo.liceses.list()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
my_repo.liceses.list()
my_repo.licenses.list()

@oerc0122 oerc0122 force-pushed the add-invenio-repo-class branch from fa03559 to 2c26c5f Compare April 29, 2025 08:30
@ElliottKasoar
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants