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

Allow iterables of qualified class names in Stream.__getitem__ searches #1359

Merged
merged 6 commits into from
Aug 12, 2022
Merged

Allow iterables of qualified class names in Stream.__getitem__ searches #1359

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f423601
Allow iterables of qualified class names in `Stream.__getitem__` sear…
jacobtylerwalls Aug 6, 2022
c5c15e0
Use implicit tuple
jacobtylerwalls Aug 8, 2022
e065756
Merge branch 'master' into gebc-multiple-classes
mscuthbert Aug 10, 2022
f4aa42c
Review edits
jacobtylerwalls Aug 11, 2022
dcfe23d
Merge branch 'master' into gebc-multiple-classes
jacobtylerwalls Aug 11, 2022
6155e9d
add type changes
mscuthbert Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions music21/stream/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,17 @@ def __getitem__(self, k: slice) -> t.List[M21ObjType]:
@overload
def __getitem__(
self,
k: t.Type[ChangedM21ObjType]
k: t.Union[t.Type[ChangedM21ObjType], t.Collection[t.Type[ChangedM21ObjType]]]
) -> iterator.RecursiveIterator[ChangedM21ObjType]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this properly type the output for mypy? (just want to be sure that that was checked)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm not quite following -- can you explain what you mean? (mypy passes, and I didn't touch the return type)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is something like, say you start typing:

for el in s[note.Note, chord.Chord]:
    print(el.pitches)
    print(el.recurse())

will the el.pitches line start to auto-suggest .pitches when you've typed .pi... (like does it know that all possible members have .pitches in it), and will mypy choke on the el.recurse() -- knowing that the union of note.Note and chord.Chord does not have .recurse() defined on all members -- or even one member.

And if you do:

for el in s[note.Note, chord.Chord]:
    if not isinstance(el, note.Note):
         print(el.closedPosition())

and know that this is fine? I'm just wondering if the typing is inferred from the ChangedM21Type.

And most importantly, does this still raise a mypy error:

for el in s[note.Note]:
     print(el.isMajorTriad())

while this passes?

for el in s[note.Note]:
     print(el.pitch)

I'll just grab the PR and play with it. But I'm wondering if putting the two different types of things in the same @overload might cause problems.

x = t.cast(iterator.RecursiveIterator[ChangedM21ObjType], self.recurse())
return x # dummy code

def __getitem__(self,
k: t.Union[str, int, slice, t.Type[ChangedM21ObjType]]
k: t.Union[str,
int,
slice,
t.Type[ChangedM21ObjType],
t.Collection[t.Type[ChangedM21ObjType]]]
) -> t.Union[iterator.RecursiveIterator[M21ObjType],
iterator.RecursiveIterator[ChangedM21ObjType],
M21ObjType,
Expand Down Expand Up @@ -486,7 +490,7 @@ def __getitem__(self,
3.0


If a class is given then an iterator of elements
If a class (or iterable of classes) is given, then an iterator of elements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to demo these two things separately. If a class is given, then a RecursiveIterator of elements matching the class is returned, similar...

then later, "Multiple classes can be passed in separated by commas. Any element matching any of the classes will be returned." with the demo of that.

that match the requested class(es) is returned, similar
to `Stream().recurse().getElementsByClass()`.

Expand All @@ -496,6 +500,8 @@ def __getitem__(self,
2
>>> len(s[note.Note])
6
>>> len(s[[note.Note, note.Rest]])
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
8

>>> for n in s[note.Note]:
... print(n.name, end=' ')
Expand Down Expand Up @@ -556,7 +562,8 @@ def __getitem__(self,

>>> s[0.5]
Traceback (most recent call last):
TypeError: Streams can get items by int, slice, class, or string query; got <class 'float'>
TypeError: Streams can get items by int, slice, class, class iterable, or string query;
got <class 'float'>

Changed in v7:
- out of range indexes now raise an IndexError, not StreamException
Expand All @@ -573,6 +580,7 @@ def __getitem__(self,
.recurse().getElementsByClass to get the earlier behavior. Old behavior
still works until v9. This is an attempt to unify __getitem__ behavior in
StreamIterators and Streams.
- allowed iterables of qualified class names, e.g. `[note.Note, note.Rest]`
'''
# need to sort if not sorted, as this call may rely on index positions
if not self.isSorted and self.autoSort:
Expand Down Expand Up @@ -607,7 +615,12 @@ def __getitem__(self,

return t.cast(M21ObjType, searchElements[k])

elif isinstance(k, type) and issubclass(k, base.Music21Object):
elif all(
isinstance(maybeType, type) and issubclass(maybeType, base.Music21Object)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
for maybeType in (
k if common.isIterable(k) else [k] # type: ignore
)
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not such a fan of this type of generator predicate. Clearer to make two separate elif statements, one for the single class and second one for the isIterable case. Students read this code and emulate it.

return self.recurse().getElementsByClass(k)

elif isinstance(k, str):
Expand All @@ -619,7 +632,8 @@ def __getitem__(self,
return querySelectorIterator

raise TypeError(
f'Streams can get items by int, slice, class, or string query; got {type(k)}'
'Streams can get items by int, slice, class, class iterable, or string query; '
f'got {type(k)}'
)

def first(self) -> t.Optional[M21ObjType]:
Expand Down