You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "giftig (via GitHub)" <gi...@apache.org> on 2023/10/31 14:07:54 UTC
[I] Technical bug: Incorrect type annotations in some superset.typing types [superset]
giftig opened a new issue, #25810:
URL: https://github.com/apache/superset/issues/25810
The `type` field has the incorrect type annotation (ironically) for at least `ResultSetColumnType` and `SQLAColumnType`. I'm not sure why `mypy` hasn't identified this as a problem.
This has the effect of confusing developers as to which types should be expected here, and has caused some type confusion / type coercion to spread around the db specs in some cases as we end up with an unexpected mix of strings and SQLA types.
#### How to reproduce the bug
- Check the actual types observed in a `db_engine_spec` class when handling either `SQLAColumnType` or `ResultSetColumnType` types, and compare to the annotation for the `type` field on these classes in both cases.
- The types are annotated as `Optional[str]` but are actually an SQLA type, not strings
- I suspect they're also not really optional and we're expecting them to always be present, but that's a little harder to confirm
### Expected results
see strings
### Actual results
see SQLA types
### Environment
(please complete the following information):
- browser type and version:
- superset version: `superset version`
- python version: `python --version`
- node.js version: `node -v`
- any feature flags active:
### Checklist
Make sure to follow these steps before submitting your issue - thank you!
- [x] I have checked the superset logs for python stacktraces and included it here as text if there are any.
- [x] I have reproduced the issue with at least the latest released version of superset.
- [x] I have checked the issue tracker for the same issue and I haven't found one similar.
### Additional context
Note I'm not aware of any bugs impacting the application as a result of this; its impact is primarily technical and resulting in confusion and/or technical mess.
--
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: notifications-unsubscribe@superset.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [I] Technical bug: Incorrect type annotations in some superset.typing types [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on issue #25810:
URL: https://github.com/apache/superset/issues/25810#issuecomment-1787295468
If I get a moment I'll raise a PR to change the type annotations but it might be worth investing some extra effort into figuring out why `mypy` didn't care that we haven't been conforming to the stated types, since it somewhat defeats the purpose of maintaining them if it doesn't.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [I] Technical bug: Incorrect type annotations in some superset.typing types [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on issue #25810:
URL: https://github.com/apache/superset/issues/25810#issuecomment-1791497830
cc @john-bodley @betodealmeida @villebro for situational awareness.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [I] Technical bug: Incorrect type annotations in some superset.typing types [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on issue #25810:
URL: https://github.com/apache/superset/issues/25810#issuecomment-1791527199
Thanks for flagging this @giftig. Whilst working on https://github.com/apache/superset/pull/25844 today I first changed the only the function signature and then ran `tox -e pre-commit` and was surprised to see that Mypy passed even though there were functions which were calling the `BaseDAO.delete()`with a single item (as opposed to a list). There's definitely something a miss.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [I] Technical bug: Incorrect type annotations in some superset.typing types [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on issue #25810:
URL: https://github.com/apache/superset/issues/25810#issuecomment-2040073344
Is anyone interested in working on this, or at least able to confirm it's an issue in 4.0 ? I assume it is, but the issue is at risk of being closed as stale if it's not heading anywhere.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org