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
DM-28646 Introduce factory for creating lightweight butlers #179
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.
Looks good, few minor comments.
d8bb948
to
8d01c89
Compare
exporter.saveDatasets(exports) | ||
|
||
# Look for any defined collection, if not get the defaults | ||
if (collections := collections) is 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.
collections := collections
? Could just check for 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.
Good catch, that was some version archeology
butlerModifier: Optional[Callable[[Butler], Butler]] = None, | ||
collections: Optional[Iterable[str]] = None | ||
) -> None: | ||
r"""buildLightweightButler is an object that is responsible for exporting |
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.
an object
-> a method
?
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.
a function
even
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.
Re-reviewing, looks good, couple of minor comments.
Introduce a factory class that is initialized with a butler that can be used to convert QuantumGraphs into lightweight sqlite backed butlers containing only the info from the graph.
8d01c89
to
a99c383
Compare
Introduce a factory class that is initialized with a butler that
can be used to convert QuantumGraphs into lightweight sqlite
backed butlers containing only the info from the graph.