Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: supporting jinja templating in saved metrics #15502

Merged
merged 4 commits into from Jul 5, 2021
Merged

feat: supporting jinja templating in saved metrics #15502

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81627b6
feat: supporting jinja templating in saved metrics
Jul 1, 2021
9b88e0e
cutting the line length to fit requirements
Jul 1, 2021
843c318
follwing PR I should have not created a new insance method
Jul 5, 2021
885e321
running precommit locally and fixing the issue
Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion superset/connectors/sqla/models.py
View file
Open in desktop
Expand Up @@ -404,9 +404,14 @@ class SqlMetric(Model, BaseMetric):
)
export_parent = "table"

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)
)
Copy link
Member

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.

Suggested change
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)

Copy link
Contributor Author

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

Copy link
Member

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.

return self.table.make_sqla_column_compatible(sqla_col, label)

@property
Expand Down