-
Notifications
You must be signed in to change notification settings - Fork 244
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
1647 pedestrian city vs street accident widget #1726
base: dev
Are you sure you want to change the base?
1647 pedestrian city vs street accident widget #1726
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1726 +/- ##
==========================================
- Coverage 53.63% 52.07% -1.57%
==========================================
Files 56 56
Lines 6272 6316 +44
==========================================
- Hits 3364 3289 -75
- Misses 2908 3027 +119
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
The logic and the code are good, the only the thing that i am not sure about is the is_urban
- I don't know if we have any other widgets that are urban, and how they are working(if they exist). - maybe someone else will know.
@assafdayan |
As at the time of implementation there was no rule based engine (is there now?) I wanted to refrain from checking location type in each widget as inter-urban and urban have mutually exclusive fields in the location. Once we have rules in place it can be removed. |
is_urban = is_location_inner_city(request_params.location_info) | ||
for w in get_widget_factories(): | ||
widget: Widget = w(request_params) | ||
if is_urban != widget.is_urban(): | ||
continue |
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.
Urban is the first categorization of a newsflash by which a widget decides whether it is relevant. Soon we will have many additional categories. The different categories should not appear on the infrastructure level. I have two suggestions until the ranking will be operational:
1 - Simply return empty items, and generate_widgets()
will not enter it to the results
2 - instead of is_urban(location_info)
, use is_included(request_params)
This wizard (as far as I can tell) is the first one implemented that requires an urban location (street rather than road and yishuv name).
I am assuming that the type of location affects which wizards are relevant. I added a boolean method to Wizard class is_urban and considered its relation to the newsflash location type in generate_widgets.
I then went ahead to implement the widget in accordance with its reddish specification and verified I got the same numbers.
closes #1647