-
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
Refactor registrar into multiple files #92
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.
I've suggested a couple specific and simple changes, but additionally we should probably think a little more about the organization. One possibility would be to make a base class - call it RegistryTable
for now - which your new classes would subclass. RegistryTable
would have base implementations for at least modify
and delete
. register
is generally more complicated and different from one table to the next, so perhaps RegistryTable.register( )
would always have to be overridden by the subclass. To the extent that the different tables need, even partially, the same treatment, the base class would be a convenient place to put the common parts of the implementations.
…asses inherit. This means common functions between the tables, e.g., delete, can be inherited from the base class
I have updated the structure as suggested. Now there is a base table class which the execution/dataset/etc tables will inherit. The base class holds the The The only slightly dirty bit is that I have to pass the execution subclass into the dataset subclass in the |
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 looks good to me. I made one trivial comment.
Add dataset status and move/delete/archive information
Add ability to delete datasets
In preparation for us doing more with entries other than creating them, I've reformatted the registrar class into one for each table, e.g., dataset, execution and dataset_alias.
As before they all follow the same format with a slight tweak. e.g.,
datareg.Registrar.register_dataset()
now becomesdatareg.Registrar.dataset.create()
anddatareg.Registrar.execution.create()
etc.This is in preparation for new functions like
datareg.Registrar.dataset.modify()
ordatareg.Registrar.dataset.delete()
etc. Placeholders for these functions have been added.It also tidies the code a bit (mainly in prep for the new functions) so that
registrar.py
didn't become very long.Right now no internal or external functionality has changed (other than the name of the function to call for registering datasets).
Will update docs to reflect changes in another branch.