Resolve Cyclic Imports #1500
Resolve Cyclic Imports #1500
Conversation
…r to have use_default_answer=True Signed-off-by: Yuyutsu Rai <[email protected]>
Signed-off-by: Yuyutsu Rai <[email protected]>
Signed-off-by: Yuyutsu Rai <[email protected]>
Signed-off-by: Yuyutsu Rai <[email protected]>
app/forms/questionnaire_form.py
Outdated
| @@ -310,20 +310,25 @@ def _get_period_range_for_single_date( | |||
| date_to: Mapping[str, dict], | |||
| ) -> timedelta: | |||
| list_item_id = self.location.list_item_id if self.location else None | |||
| value_source_resolver = ValueSourceResolver( | |||
| rule_evaluator = RuleEvaluator( | |||
There was a problem hiding this comment.
I'm not sure this makes sense at the moment, the rule evaluator now has a value source resolver but you're still passing both to the field handler?
There was a problem hiding this comment.
I was not sure if it was okay to remove but now removed VSR from FieldHandler.
app/questionnaire/resolver.py
Outdated
|
|
||
|
|
||
| @dataclass | ||
| class Resolver(ABC): |
There was a problem hiding this comment.
Do we need this abstract class? I don't think this makes sense since there is only one instance of a "Resolver" class? So all of the stuff from ValueSourceResolver would be in here if we did this/nothing would be overridden since they are be the same?
There was a problem hiding this comment.
I implemented it as a pre measure and was not sure how we wanted to approach the type hinting. I have updated VSR and RuleEvaluator, where RuleEvaluator uses type annotations for the type hinting.
Signed-off-by: Yuyutsu Rai <[email protected]>
Signed-off-by: Yuyutsu Rai <[email protected]>
| @@ -480,32 +480,47 @@ def get_answer_fields( | |||
| if block_ids_by_section: | |||
| block_ids = get_flattened_mapping_values(block_ids_by_section) | |||
|
|
|||
| def _get_value_source_resolver(list_item: str | None = None) -> ValueSourceResolver: | |||
| def _get_value_source_resolver( | |||
There was a problem hiding this comment.
There are still some inconsistencies taking this approach, as we seem to mix where we call rule evaluator and where we are calling value source resolver directly. For example here, if we were to do this we'd be better of getting the rule_evaluator as it already has a value source resolver.
There was a problem hiding this comment.
Probably worth having a chat about this approach with the team anyway.
| @@ -480,32 +480,47 @@ def get_answer_fields( | |||
| if block_ids_by_section: | |||
| block_ids = get_flattened_mapping_values(block_ids_by_section) | |||
|
|
|||
| def _get_value_source_resolver(list_item: str | None = None) -> ValueSourceResolver: | |||
| def _get_value_source_resolver( | |||
There was a problem hiding this comment.
If this private method does not use vars from the higher scope of the class implicitly (is only passing parameters explicitly), there is no reason why to nest it inside and in the middle of another method of this class. Should be separated out from the parent method.
There was a problem hiding this comment.
I think it still uses the higher scope of the function. If not I think the function would not be needed
| @@ -50,6 +45,7 @@ def __post_init__(self) -> None: | |||
| self.operations = Operations( | |||
| language=self.language, schema=self.schema, renderer=renderer | |||
| ) | |||
| self.value_source_resolver = self.value_source_resolver | |||
There was a problem hiding this comment.
Does this statement make sense?
There was a problem hiding this comment.
Technically, yes 😂 . Removed 👍
| @@ -272,6 +272,14 @@ def _resolve_summary_with_calculation( | |||
| data_stores=self.data_stores, | |||
| location=self.location, | |||
| routing_path_block_ids=self.routing_path_block_ids, | |||
| value_source_resolver=ValueSourceResolver( | |||
There was a problem hiding this comment.
We definitely don't want to instantiate another ValueSourceResolver inside the ValueSourceResolver class instance.
There was a problem hiding this comment.
I agree with @berroar this probably needs some more thought and some kind of structured approach/plan/proposal.
There was a problem hiding this comment.
Yeah this felt akward to do when refactoring, Will have a look
| location=this_location, | ||
| routing_path_block_ids=block_ids_for_dependencies, | ||
| value_source_resolver=ValueSourceResolver( |
There was a problem hiding this comment.
We could probably call the same instance of VSR here and on line 245, if we initialised VSR when the PathFinder class instance is first created, but that requires some more thought as we would need to refactor how we initialise PathFinder class.
There was a problem hiding this comment.
Will bring this up in Tech Session once everyone has had a look
There was a problem hiding this comment.
Great work 👍 I agree though that we might want to take a bit more time and look at some of the alternatives like breaking down some of the bigger classes into smaller ones to get around the cyclic imports. Appreciate this is a lot more complex though and might not be as straightforward as that. Maybe a spike ticket to look at some possible solutions, with this being one of them?
|
Yeah I agree, as this approach still has its issues and would be better if we can find another approach. - Closing PR |
|
NEW Investigation card: https://jira.ons.gov.uk/browse/ECI-1498 |
What is the context of this PR?
This PR refactors VSR and RuleEvaluator to resolve most of the Cyclic imports found after upgrading Pylint to version
v3.2.5.How to review
Read through the changes and check if runner works as expected.
Check if tests pass locally
Checklist