-
Notifications
You must be signed in to change notification settings - Fork 28
[uss_qualifier] allow PlanningAreaResource to be imported via old path #1051
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
Conversation
2a2b2d9 to
e847075
Compare
e847075 to
d88da2c
Compare
mickmis
left a comment
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.
Have you considered something like this instead?
import warnings
from monitoring.uss_qualifier.resources import PlanningAreaResource as PlanningAreaResourceOrig, PlanningAreaSpecification
class PlanningAreaResource(PlanningAreaResourceOrig):
def __init__(self, specification: PlanningAreaSpecification, resource_origin: str):
super(PlanningAreaResource, self).__init__(specification, resource_origin)
warnings.warn(
"PlanningAreaResource has moved from 'resources.astm.f3548.v21' to 'resources'. Importing it from its current location is deprecated and will be removed in the future.",
UserWarning,
stacklevel=2,
)| @@ -1,9 +1,8 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import datetime | |||
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.
About SubscriptionParams: actually it looks like this is not a resource. This is never declared as such in a yaml file, nor used as a field in one. I feel like this may be better suited in monitorlib. WDYT?
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.
moving to monitorlib indeed makes sense, will do
That would be simpler indeed. However, I just tried and I get this error: I guess the subclassing breaks an equality check, while the current approach ensures that the types are the exact same. |
93069f7 to
ae17102
Compare
ae17102 to
9321172
Compare
9321172 to
85270a6
Compare
mickmis
left a comment
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.
I find this approach a bit barbarian but I can't find a better alternative. Let's hope it also does not have strange side effects. So let's go ahead with it :)
This prevents #1048 from breaking existing configurations.
Notes
Circular imports
Fixing required to move the
subscription_params.pyfile to a separate module, otherwise we run into a circular import problem (the placeholder imports the class from the new path, but the class itself still imports the subscription_params).The
paramsmodule is a sibling ofv21: making it a child module ofv21still yields the circular import error:ImportError: cannot import name 'PlanningAreaResource' from partially initialized module 'monitoring.uss_qualifier.resources' (most likely due to a circular import) (/app/monitoring/uss_qualifier/resources/__init__.py)User warnings
I tested that the import of the old resource in a configuration triggers the warning. With a single usage of the resource in f3548_self_contained, we get (see this test run):
If no configuration references the resource, we have no warning.