-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
fix: add schema name to datasource field in chart list #11278
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11278 +/- ##
==========================================
- Coverage 65.43% 55.96% -9.48%
==========================================
Files 831 404 -427
Lines 39528 13410 -26118
Branches 3598 3392 -206
==========================================
- Hits 25866 7505 -18361
+ Misses 13553 5723 -7830
- Partials 109 182 +73
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.
thanks for the quick fix! one comment (non-blocking)
superset/models/slice.py
Outdated
@@ -140,9 +140,15 @@ def datasource_url(self) -> Optional[str]: | |||
def datasource_name_text(self) -> Optional[str]: | |||
# pylint: disable=no-member | |||
if self.table: | |||
if self.table.schema: | |||
return f"{self.table.schema}.{self.table.table_name}" | |||
return self.table.table_name | |||
datasource = self.datasource |
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.
any reason why we have this here if we're not handling self.table
the same way?
I don't really care which we do, but let's be consistent (either table
and datasource
or self.table
and self.datasource
)
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.
looks like most methods in this model share this inconsistency 😕
SUMMARY
see title and linked ticket
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION