Observatory Dependency Injection #195
Conversation
* Create `Observatory` outside `POCS` class and pass in as param
Codecov Report
@@ Coverage Diff @@
## develop #195 +/- ##
===========================================
- Coverage 81.32% 81.32% -0.01%
===========================================
Files 37 37
Lines 2662 2661 -1
Branches 331 331
===========================================
- Hits 2165 2164 -1
Misses 392 392
Partials 105 105
Continue to review full report at Codecov.
|
| @@ -15,6 +15,7 @@ from astropy.io import fits | |||
| from astropy.utils import console | |||
|
|
|||
| from pocs import POCS | |||
| from pocs.observatory import Observatory | |||
There was a problem hiding this comment.
Please import package or module, not class. In this case, you'd say:
import pocs.observatory
And later:
observatory = pocs.observatory.Observatory(simulator=simulator)
There was a problem hiding this comment.
As I remarked in another PR, this was not something that we explicitly decided on, we only talked about using root relative syntax. Either style is correct and the thing that is usually stressed is to be consistent. Since 95% of the current code uses the from syntax, which is what we have been encouraging previously, I'm not sure what the arguments are for in favor of changing them all. It is slightly more obvious what package is being referred to so I could understand wanting to switch to it, but that is a conversation we have not had yet.
There was a problem hiding this comment.
I think you made a comment but disappeared. I looked at the google style code a bit more and see what they are getting at. It's not explicitly saying not to use the from style but to use just the module without prefix, which is not what I am doing. Will fix accordingly. From here
| observatory, | ||
| state_machine_file='simple_state_table', | ||
| messaging=False, | ||
| **kwargs): | ||
|
|
||
| # Explicitly call the base classes in the order we want |
There was a problem hiding this comment.
FWIW, this comment and the associated code seem to imply that we should use aggregation for PanStateMachine.
There was a problem hiding this comment.
I think this might be the comment that you missed, about the awkwardness of multiple inheritance.
| @@ -9,14 +9,24 @@ | |||
| from pocs import POCS | |||
| from pocs import _check_config | |||
| from pocs import _check_environment | |||
| from pocs.observatory import Observatory | |||
There was a problem hiding this comment.
Ditto on the import style.
| def pocs(config): | ||
| def observatory(): | ||
| observatory = Observatory(simulator=['all']) | ||
|
|
| pocs = POCS(simulator=['camera', 'mount', 'night'], | ||
| observatory.config['simulator'] = ['camera', 'mount', 'night'] | ||
|
|
||
| pocs = POCS(observatory, | ||
| messaging=True, safe_delay=15) |
There was a problem hiding this comment.
FWIW, looks like safe_delay is one of several undocumented parameters to POCS.
Simple change to create the
Observatoryoutside of POCS and require it as a first parameter.This will allow for a lot easier use by Huntsman and other observatories.