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: supporting jinja templating in saved metrics #15502
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15502 +/- ##
==========================================
- Coverage 77.20% 76.82% -0.38%
==========================================
Files 975 976 +1
Lines 50842 51306 +464
Branches 6728 6912 +184
==========================================
+ Hits 39251 39417 +166
- Misses 11376 11670 +294
- Partials 215 219 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset/connectors/sqla/models.py
Outdated
def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: | ||
return self.table.get_template_processor() | ||
|
||
def get_sqla_col(self, label: Optional[str] = None) -> Column: | ||
label = label or self.metric_name | ||
sqla_col: ColumnClause = literal_column(self.expression) | ||
sqla_col: ColumnClause = literal_column( | ||
self.get_template_processor().process_template(self.expression) | ||
) |
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.
Thanks for your contributions. It is not recommended to add the new instance method(get_template_processor) here.
def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: | |
return self.table.get_template_processor() | |
def get_sqla_col(self, label: Optional[str] = None) -> Column: | |
label = label or self.metric_name | |
sqla_col: ColumnClause = literal_column(self.expression) | |
sqla_col: ColumnClause = literal_column( | |
self.get_template_processor().process_template(self.expression) | |
) | |
def get_sqla_col(self, label: Optional[str] = None) -> Column: | |
label = label or self.metric_name | |
tp = self.table.get_template_processor() | |
sqla_col = literal_column(tp.process_template(self.expression)) | |
return self.table.make_sqla_column_compatible(sqla_col, label) |
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.
Got it,
Fixed the code,
Why it is not recommended to have new instance method? I am asking only yo expand my knowledge
Thanks
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.
Hi Guy. The main reason is no more self.get_template_processor
method call currently, so no need to extend this class. If there is a need for this in the future, we can easily extend it.
closes: #6983 |
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.
LGTM! Thanks!
* feat: supporting jinja templating in saved metrics * cutting the line length to fit requirements * follwing PR I should have not created a new insance method * running precommit locally and fixing the issue Co-authored-by: Guy <guy@nexite.io>
* feat: supporting jinja templating in saved metrics * cutting the line length to fit requirements * follwing PR I should have not created a new insance method * running precommit locally and fixing the issue Co-authored-by: Guy <guy@nexite.io>
* feat: supporting jinja templating in saved metrics * cutting the line length to fit requirements * follwing PR I should have not created a new insance method * running precommit locally and fixing the issue Co-authored-by: Guy <guy@nexite.io>
SUMMARY
Adding the ability to use jinja templating also in saved metrics.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION