sublimehq / sublime_text

Issue tracker for Sublime Text
https://www.sublimetext.com
810 stars 39 forks source link

[Docs] Wrong type for Selection.add_all #5809

Open rchl opened 1 year ago

rchl commented 1 year ago

Description of the bug

Selection.add_all can also take a Point since it calls Selection.add which takes a point.

    def add(self, x: Region | Point):
        """
        Add a `Region` or `Point` to the selection. It will be merged with the
        existing regions if intersecting.
        """
        if isinstance(x, Region):
            sublime_api.view_selection_add_region(self.view_id, x.a, x.b, x.xpos)
        else:
            sublime_api.view_selection_add_point(self.view_id, x)

    def add_all(self, regions: Iterator[Region]):
        """ Add all the regions from the given iterable. """
        for r in regions:
            self.add(r)

And also, Iterator should be Iterable since List is not a subset of Iterator due to missing the __next__ implementation so passing plain list to add_all will show a type error.

I believe the type should thus be Iterable[Region | Point]

Sublime Text build number

4147

Operating system & version

macOS

rchl commented 1 year ago

Same issue I've just noticed in PhantomSet.update method. It's def update(self, phantoms: Iterator[Phantom]): , should be def update(self, phantoms: Iterable[Phantom]):

rchl commented 1 year ago

Should be a a quick fix.

rchl commented 1 year ago

BTW. It's not only the add_all method. Also update:

Screenshot 2023-09-14 at 08 34 27

And while at it, this also jumps out as wrong:

Screenshot 2023-09-14 at 08 34 18

(Though if we'd apply the strict Pyright checking then there would be much more to report...)

BenjaminSchaaf commented 1 year ago

Fixed in build 4157.

rchl commented 1 year ago

The type of region in Selection.add_all was updated to Iterator[Region | Point] but as I've mentioned in this issue, it's not an Iterator but Iterable. A plain list is not compatible with Iterator type so you can't pass it to add_all which is clearly supported.

And also the same issue applies to: PhantomSet.update.

And the [Phantom] in self.phantoms: [Phantom] = [] is an invalid annotation. Should be List[Phantom]. Unless it is allowed in some latest Python versions but still the more compatible way would be using List.