Update orderinglist annotations, list compatibity #10889
Conversation
- Don't omit the `_T` typevar in `ordering_list` and `OrderingList.ordering_func``; type checkers need to understand the relationship between the `OrderingList` instance and the ordering function connected to it. - The ordering function can return _any_ value, not just integers - The `ordering_attr` argument to `OrderingList` is not optional - Update list methods to accept the same signature as the overridden methods, including `SupportsIndex` instead of `int`, an iterable of `_T` when using `__setitem__` with a slice (and not just sequences) and converting the index value to an integer before passing it to the `ordering_func` callable. - Update `__setitem__` to _not_ attempt to handle slice objects as handling all edge cases of slice length and iterable length is very tricky and most use of `OrderingList` and slices is handled by the SQLAlchemy collections instrumentation anyway. - Remove the `__setslice__` and `__delslice__` methods, which were deprecated in Python 2.6 and removed in Python 3.0. Fixes sqlalchemy#10888
|
For context: this started as a quick fix for the |
|
are we going to add some typing tests ? these can go into https://github.com/sqlalchemy/sqlalchemy/tree/main/test/typing/plain_files/ext where we probably could have a new orderinglist.py module and/or folder. |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 314dbc6 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 314dbc6: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5118 |
|
Michael Bayer (zzzeek) wrote: good stuff, dont worry about that oracle failure. can we add a changelog file here that we applied full typing to orderinglist? View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5118 |
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5118
| # is far simpler. | ||
| # SQLAlchemy collection instrumentation otherwise takes care of | ||
| # slices in all normal use of OrderingList in ORM classes. | ||
| super().__setitem__(__index, __entity) |
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
is there a test that would fail with the old implementation? I missed that the original issue refers to multiple problems, some that are not typing. the changelog note should include this as well, likely as a separate changelog item completely (can still be in one file, multiple `.. change::`` directives can be in one file
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5118
There was a problem hiding this comment.
Your implementation demonstrates a clear understanding of advanced SQLAlchemy features, such as the ordering_list extension and custom ordering functions. The use of text_to_pos as an ordering function within pos_from_text effectively extracts numerical positions from Bullet text, showcasing your ability to handle complex ordering logic. The Slide and Bullet classes are well-defined, with proper relationships and use of mapped columns. Including the type-checking section with reveal_type ensures that the types of pos_from_text and slide.bullets are correctly inferred, which is crucial for maintaining type safety in your code. Overall, this code is a sophisticated and well-implemented example of leveraging SQLAlchemy's capabilities for ordered collections.
Description
_Ttypevar inordering_listandOrderingList.ordering_func; type checkers need to understand the relationship between theOrderingListinstance and the ordering function connected to it.ordering_attrargument toOrderingListis not optionalSupportsIndexinstead ofint, an iterable of_Twhen using__setitem__with a slice (and not just sequences) and converting the index value to an integer before passing it to theordering_funccallable.__setitem__to not attempt to handle slice objects as handling all edge cases of slice length and iterable length is very tricky and most use ofOrderingListand slices is handled by the SQLAlchemy collections instrumentation anyway.__setslice__and__delslice__methods, which were deprecated in Python 2.6 and removed in Python 3.0.Fixes #10888
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit message