-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
feat(jinja): metric macro #27582
feat(jinja): metric macro #27582
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #27582 +/- ##
==========================================
+ Coverage 67.46% 69.85% +2.38%
==========================================
Files 1910 1911 +1
Lines 74802 74982 +180
Branches 8345 8345
==========================================
+ Hits 50467 52375 +1908
+ Misses 22284 20556 -1728
Partials 2051 2051
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c28cb24
to
dac25ac
Compare
if chart: | ||
return chart.datasource_id | ||
if dataset_id := form_data.get("url_params", {}).get("datasource_id"): | ||
return dataset_id |
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.
this could be None
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.
that's a good catch! I was thinking that you must associate a dataset to a chart when creating one, but forgot it could be deleted after. Improved that if chart
to if chart and chart.datasource_id
to avoid returning None
.
On the second part tho (with the walrus operator) if url_params["datasource_id"]
is None
, this if
block condition won't be met and therefore it won't return. I added a test case covering this specific scenario.
if chart_id := ( | ||
form_data.get("slice_id") or form_data.get("url_params", {}).get("slice_id") | ||
): | ||
chart_data = ChartDAO.find_by_id(chart_id) |
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.
chart_id
could be None
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.
Similarly, the walrus operator condition here shouldn't be met so we wouldn't return None
. Also added a test case for this scenario.
(cherry picked from commit d874225)
(cherry picked from commit d874225)
(cherry picked from commit d874225)
(cherry picked from commit d874225)
(cherry picked from commit d874225)
(cherry picked from commit d874225)
(cherry picked from commit d874225)
SUMMARY
This PR adds a new Jinja macro:
{{ metric('metric_key', dataset_id) }}
. This macro can be used to retrieve metric syntax from a dataset, to be re-used in charts/SQL Lab/etc.This is useful to avoid copy/paste and provide a centralized place for metric management. Some use-cases:
{{ metric('revenue') }} - {{ metric('cost') }}
.The
dataset_id
parameter is optional and if not provided Superset will attempt to get it from the current context.I also noticed there were both
test_jinja_context.py
andjinja_context_test.py
, so I unified the two.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
Video attached to Google Drive (Exceeds 10MB)
TESTING INSTRUCTIONS
Both unit and integration tests added. For manual testing:
{{ metric("metric_key") }}
, replacingmetric_key
with the key used to create the metric in step 1.ADDITIONAL INFORMATION
ENABLE_TEMPLATE_PROCESSING
.