villebro commented on a change in pull request #18998:
URL: https://github.com/apache/superset/pull/18998#discussion_r818993921
##########
File path: superset/utils/core.py
##########
@@ -1097,11 +1097,13 @@ def merge_extra_form_data(form_data: Dict[str, Any]) ->
None:
{"isExtra": True, **fltr} for fltr in append_adhoc_filters # type:
ignore
)
if append_filters:
- adhoc_filters.extend(
- simple_filter_to_adhoc({"isExtra": True, **fltr}) # type: ignore
- for fltr in append_filters
- if fltr
- )
+ for key, value in form_data.items():
+ if re.match("adhoc_filter.*", key):
+ value.extend(
+ simple_filter_to_adhoc({"isExtra": True, **fltr}) # type:
ignore
+ for fltr in append_filters
+ if fltr
+ )
Review comment:
@ktmud in offline discussions I was the person who proposed using a
regex to loop through this, so don't blame @kgabryje for this 😄 My reasoning
went something like this:
1) it's fairly uncommon for there to be more than 20 controls. So
performance impact of looping through all control keys is negligible
2) While I know that this is put here mostly for the Mixed Timeseries chart,
I don't see any reason why someone wouldn't consider having 3 (or more) query
sections. Only supporting one additional control called `adhoc_filters_b`, but
not, say, `adhoc_filters_c` or `adhoc_filters_2`, feels slightly arbitrary. The
logic behind my reasoning here was, "any control that starts with
`adhoc_filters` should be considered an adhoc filter control, and extra filters
should apply to them all". While both solutions solve this specific problem, I
still feel the regex solution is less coupled to the Mixed Timeseries chart
than only matching `["adhoc_filters", "adhoc_filters_b"]`
Having said that, this was really just a minor nit and likely irrelevant in
the grand scheme of things, so I won't get hung up on this either way 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
