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.

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

bpy_prop_collection.get() returns None #184

Closed
Road-hog123 opened this issue Feb 7, 2024 · 10 comments
Closed

bpy_prop_collection.get() returns None #184

Road-hog123 opened this issue Feb 7, 2024 · 10 comments
Assignees
Labels

Comments

@Road-hog123
Copy link
Contributor

System Information

  • OS: Windows 10
  • fake_bpy_modules_4.0-20231118

Expected behavior

A method named get() should return a value.

Description about the bug

Mesh.uv_layers is a bpy_prop_collection[MeshUVLoopLayer], so retrieving an element by name with .get() should return either a MeshUVLoopLayer or the type of the default parameter.

typing.Any would be an improvement over None.

I note that foreach_get(), items() and values() could also benefit from GenericType, however get() is complicated by its default parameter.

@nutti
Copy link
Owner

nutti commented May 17, 2024

I fixed the issues mentioned by @JonathanPlasse.

@Road-hog123 @JonathanPlasse

I'm not sure how to foreach_get().
What is the proper data type for it?

@nutti nutti self-assigned this May 17, 2024
@nutti nutti added the bug label May 17, 2024
@Road-hog123
Copy link
Contributor Author

Mesh.uv_layers is a bpy_prop_collection[MeshUVLoopLayer], so retrieving an element by name with .get() should return either a MeshUVLoopLayer or the type of the default parameter.

As per my comment in #153 the fixed return type of Optional[GenericType] is incorrect when default is not None.

I'm not sure if I should open a separate issue for it, but the key parameters of find() and get() should be str not str | None.

I'm not sure how to foreach_get(). What is the proper data type for it?

foreach_get and foreach_set are problematic because of multi-dimensional array flattening—probably best to open a separate issue for them specifically.

@nutti
Copy link
Owner

nutti commented May 18, 2024

@Road-hog123

foreach_get and foreach_set are problematic because of multi-dimensional array flattening—probably best to open a separate issue for them specifically.

No prob. You can also discuss this here.
Before implementing this to our generation code, we need to fix the correct data type for foreach_get and foreach_set at first.
Actually, we could not cover all use-cases of this data type, your comments are very useful for us.

@Road-hog123
Copy link
Contributor Author

My initial thoughts are:
foreach_get(attr: str, seq: MutableSequence) -> None
foreach_set(attr: str, seq: Sequence[bool | int | float]) -> None

These are very C-like methods, with foreach_get() trying to avoid memory allocation by writing to an existing sequence—for type-checking purposes I think it just needs to be mutable and support __len__(). As foreach_set() reads from seq it doesn't need to be mutable, but it does still need to support __len__().

The contained type of seq depends on the value of attr—if getattr(GenericType, attr) returns bool then seq should be Sequence[bool], if Vector then it should be Sequence[float] (and the sequence length is multiplied by the number of dimensions the vector has, which doesn't matter for type checking). I don't know if a type checker could actually determine the correct type, but as it will always be one or more of bool, int or float I think a union is a good starting point.

@nutti
Copy link
Owner

nutti commented May 18, 2024

@Road-hog123

I think MutableSequence is now deprecated.
Is it possible to use list instead?

@Road-hog123
Copy link
Contributor Author

typing.MutableSequence and collections.abc.ByteString are deprecated, but collections.abc.MutableSequence shouldn't be?

@nutti
Copy link
Owner

nutti commented May 19, 2024

@Road-hog123 @JonathanPlasse

Could you give me an advice about this type annotation?
Both typing.Sequence[bool] | typing.Sequence[int] | typing.Sequence[float] and typing.Sequence[bool | int | float] will be an error while executing ruff formatter.
What is a proper way to these annotation?
Also, collections.abc.MutableSequence[bool] | collections.abc.MutableSequence[int] | collections.abc.MutableSequence[float] and collections.abc.MutableSequence[bool | int | float] will be an error.

@Road-hog123
Copy link
Contributor Author

collections.abc.MutableSequence[bool] | collections.abc.MutableSequence[int] | collections.abc.MutableSequence[float] and collections.abc.MutableSequence[bool | int | float] will be an error.

What is the error? I don't see why either of these would be incorrect type annotations.

On further consideration the former is more correct for these methods as the sequence should be homogenous.

@nutti
Copy link
Owner

nutti commented May 19, 2024

What is the error? I don't see why either of these would be incorrect type annotations.

This is my mistake. I have already solved this.

The issue is now fixed. Thanks for sharing your idea @Road-hog123 !

@nutti nutti closed this as completed May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants