Skip to content

Fix type of LabelClasses.classes #618

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.

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

Closed
lossyrob opened this issue Sep 2, 2021 · 2 comments
Closed

Fix type of LabelClasses.classes #618

lossyrob opened this issue Sep 2, 2021 · 2 comments

Comments

@lossyrob
Copy link
Member

lossyrob commented Sep 2, 2021

Currently setting a LabelClasses.classes property to List[str] gives an error in Pylance, e.g.

classes: List[str] = ...
label = LabelExtension.ext(item, add_if_missing=True)
label.apply(
    label_description="USGS GAP/LANDFIRE",
    label_type=LabelType.RASTER,
    label_classes=[
        LabelClasses.create(
            classes=classes,
            name="",
        )
    ],
)

yields error at classes=classes with

Argument of type "list[str]" cannot be assigned to parameter "classes" of type "List[str | int | float]" in function "create"
  TypeVar "_T@list" is invariant
    Type "str" cannot be assigned to type "str | int | float"
      "str" is incompatible with "int"
      "str" is incompatible with "float"

This I believe is due to the Union type as the generic to List here and here

This should be Union[List[str], List[int], List[float]] instead.

Bonus: search for other instances of List[Union[...]] and convert to Union[List[.], List[.], ...] to avoid a similar issue if it exists elsewhere.

@cuttlefish
Copy link
Contributor

cuttlefish commented Sep 2, 2021

Is there a reason to use List over Sequence here? It probably doesn't make sense to take mutable data types as inputs?
I believe that Sequence can maintain the heterogeneity of the datatype (which may not be desired in this specific case, but would be useful elsewhere).
I've got a branch with the Label extension converted to use Sequence if there's any interest. This would probably only make sense if we were intending to transition everything over to Sequence.

@duckontheweb
Copy link
Contributor

Closed via #627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants