DM-46165: Removed optical configuration from enums #56
Conversation
There was a problem hiding this comment.
I think this looks mostly ok. I do have one thing I hope you will reconsider before merging. Instead of duplicating the enumeration definition all over the place, continue to use it from the package enums.py module (optionally re-exporting from ts-xml when it is available) and just add a note to remove it from there later.
| @@ -46,7 +46,7 @@ | |||
| type: number | |||
| optical_configuration: | |||
| description: The mirror alignment configuration for the laser | |||
| enum: ["SCU","No SCU","F1 SCU","F1 No SCU","F2 SCU","F2 No SCU"] | |||
| enum: ["SCU","F1 SCU","F2 SCU","No SCU","F1 No SCU","F2 No SCU"] | |||
There was a problem hiding this comment.
Consider making this dynamic by using the enumeration to generate the valid entries:, e.g.;
CONFIG_SCHEMA = yaml.safe_load(
f"""
...
enum: {[e.value for e in OpticalConfiguration]}
...
"""
There was a problem hiding this comment.
In order to do this, I'd have to import OpticalConfiguration. Does that work in config_schema?
| from .register import AsciiRegister | ||
|
|
||
| try: |
There was a problem hiding this comment.
you are doing this in multiple places, defining the same enumeration, and you are then removing the enum from the enums.py module.
Consider leaving this alone and just adding a "TODO" in the enums.py module to replace the enumeration with the one from ts-xml once that becomes available.
python/lsst/ts/tunablelaser/csc.py
Outdated
| from lsst.ts.xml.enums import TunableLaser | ||
| from lsst.ts.xml.enums.TunableLaser import LaserDetailedState, LaserErrorCode | ||
|
|
||
| try: |
There was a problem hiding this comment.
See my comment above about this.
| @@ -31,20 +31,3 @@ class Output(enum.StrEnum): | |||
| """A calibration energy level where the energy level adjusts.""" | |||
| MAX = "MAX" | |||
| """Maximum energy level for the laser.""" | |||
|
|
|||
|
|
|||
| class OpticalConfiguration(enum.StrEnum): | |||
There was a problem hiding this comment.
I suggest you leave this alone for now and just add a note here to remove this enum and use the one from ts-xml once that is available. The way you are going about this is a bit awkward.
If you want to automatically switch the ts-xml once it is available, you can re-export it from here adding the try/except you included in the other files here and leaving the OpticalConfiguration entry in the __all__ list above.
|
|
||
| # TODO: (DM-46168) Revert workaround for TunableLaser XML changes | ||
| try: | ||
| from lsst.ts.xml.enums.TunableLaser import OpticalConfiguration |
There was a problem hiding this comment.
See my note about this above. Just continue doing what you are doing now and replace the enum once it is available in ts-xml.
This is related to lsst-ts/ts_xml#904, where we added this to the XML Enums