DM-20763: Add initial support for Gen3 Butler to obs_decam #216
Conversation
parejkoj
force-pushed
the
tickets/DM-20763
branch
2 times, most recently
from
b432347
to
74a4957
Compare
| pass | ||
| else: | ||
| self.assertIsNotNone(self.butler.registry.find(self.butler.collection, dt, self.dataId)) | ||
| super().setUp() |
There was a problem hiding this comment.
This is a bit of a nitpick, but seeing this in action makes me think that it might be slightly better for the base class interface to be an abstractmethod that takes all of the setUp attributes as arguments, which would be called by the base-class setUp. That makes it harder for the derived class to forget something and get delayed, harder-to-interpret errors. Of course, this is just test code, so it's not like it's a case where immediate vs. delayed errors are a big deal.
There was a problem hiding this comment.
Also, since the setUp being called here is not the unittest setUp, it might be clearer to rename the method in the parent class so it's clear you are calling the IngestTestBase one and not the TestCase one.
There was a problem hiding this comment.
@timj : I don't understand what you mean by "not the unittest setUp"? It's the MRO's job to sort that out. And unittest doesn't do anything in setUp anyway.
There was a problem hiding this comment.
Why leave it to the MRO? It seems to me to be much clearer to have no doubt as to which parent method is being called. Calling it setUp gives it connotations that TestCase is involved somewhere.
There was a problem hiding this comment.
I'm still confused: this is setUp, which is only called if this is a TestCase. That's the whole point. The MRO will call whatever parent setUps are available, in the appropriate order.
There was a problem hiding this comment.
Does python really call all setUps that are available? Doesn't it only do that if the method explicitly asks the parent to be called? If you think it's not at all confusing to people which setUp is being called (but I don't think it's both) then leave it. I'm saying I have to think about which one is being called and if it was called something else then that would be clearer to me (one of the parent classes is not a TestCase).
There was a problem hiding this comment.
TestCase does not define setUp though.
There was a problem hiding this comment.
I am less concerned with the naming here and more with the fact that you are passing objects to the base setUp by setting them as attributes before the call, rather than just passing them naturally as arguments.
parejkoj
force-pushed
the
tickets/DM-20763
branch
from
4874dd9
to
0327b68
Compare
Remove HSC_FILTER_NAMES as those are now fully handled by the FilterDefintions. rawFormatter.makefilters() is now done in the base class. Add tests of Instrument
TallJimbo
force-pushed
the
tickets/DM-20763
branch
2 times, most recently
from
5913239
to
251f583
Compare
This duplicates some data, but it's still cleaner than trying to programmatically rewrite filter names, especially when you consider the fact that we'll eventually just remove half of these lines.
TallJimbo
force-pushed
the
tickets/DM-20763
branch
from
251f583
to
7a11148
Compare
No description provided.