john-bodley commented on code in PR #26200:
URL: https://github.com/apache/superset/pull/26200#discussion_r1417983462
##########
tests/unit_tests/datasource/dao_tests.py:
##########
@@ -106,7 +106,6 @@ def test_get_datasource_sqlatable(session_with_data:
Session) -> None:
result = DatasourceDAO.get_datasource(
datasource_type=DatasourceType.TABLE,
datasource_id=1,
- session=session_with_data,
Review Comment:
The `db.session` is mocked
[here](https://github.com/apache/superset/blob/master/tests/unit_tests/conftest.py#L65)
and is included as a fixture to the test.
##########
superset/utils/log.py:
##########
@@ -152,8 +153,7 @@ def log_with_context( # pylint: disable=too-many-locals
# need to add them back before logging to capture user_id
if user_id is None:
try:
- session = current_app.appbuilder.get_session
- session.add(g.user)
+ db.session.add(g.user)
Review Comment:
Same session as `current_app.appbuilder.get_session`. Consistency is key
here.
##########
superset/daos/datasource.py:
##########
@@ -45,15 +44,14 @@ class DatasourceDAO(BaseDAO[Datasource]):
@classmethod
def get_datasource(
cls,
- session: Session,
Review Comment:
It is not apparent to me why the `session` was included here. There are unit
tests which use a session other than `db.session` but, per
[here](https://github.com/apache/superset/blob/master/tests/unit_tests/conftest.py#L65),
the global `db.session` is mocked to use said session.
--
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]
