Add PacBio Data Transfer service #3477
Conversation
| class PacBioDataTransferService(PostProcessingDataTransferService): | ||
|
|
||
| def __init__(self, metrics_service: PacBioMetricsParser): | ||
| super().__init__(metrics_service=metrics_service) |
There was a problem hiding this comment.
Why are you calling the parent constructor like this? Seems wonky! 😄
I'd expect the constructor to look like
def __init__(self, metrics_service: PacBioMetricsParser):
self.metrics_service = metrics_serviceAlso, is not the parent class abstract? It does not make sense to call the constructor of an abstract class!
There was a problem hiding this comment.
I read some more and it might make sense to call the parent constructor if there is shared logic in the constructor, I'll leave it to you! I guess you want to enforce a strict design of the components. But maybe you can think about this a bit more.
I'm more used to interfaces which work a bit differently, only specifying the expected methods to be available on the class implementing the interface. A single class can also implement multiple interfaces
There was a problem hiding this comment.
We are fixing this later on.
| super().__init__(metrics_service=metrics_service) | ||
|
|
||
| def get_post_processing_dtos(self, run_data: PacBioRunData) -> PacBioDTOs: | ||
| metrics: PacBioMetrics = self.metrics_service.parse_metrics(run_data) |
There was a problem hiding this comment.
I'm guessing exceptions can be thrown from the metrics parsing, should they be caught here or just bubble up?
There was a problem hiding this comment.
we are fixing error handling in a later step
|
There was a problem hiding this comment.
Looks good! Some concerns about how you intend to dump the DTO directly into the database model, creating a tight coupling between the two. I'd keep some nestling in the DTO because it is easier to understand what it contains that way.
Might need some error handling for the metrics parsing, unless that is done elsewhere.
Let me know if you have questions, ping me in slack or here 😄
Description
Closes https://github.com/Clinical-Genomics/add-new-tech/issues/69
Add PacBio data transfer service
Added
Changed
Fixed
How to prepare for test
uspaxaHow to test
Expected test outcome
Review
Thanks for filling in who performed the code review and the test!
This version is a
Implementation Plan