I want to report some issues during testing version 2.11.0 of the specklepy.
The type checking causing issues for ‘Optional[List]’ attribute
For example, as you can see for class Element2D and LoadNode
When the attribute of ‘topology’ and ‘nodes’ are being checked, the error will occur in _validate_type even when the value is in the correct type, this is because the t does not have args attribute. These occurs in more places:
What we did to fix the error is just to add Base in the type hints as this, but maybe you guys have other ideas for the solution.
The unit for the load cannot be assigned as Newton
As you can see in the picture when the units attribute of LoadNode is set to ‘N’, it gives an error when we try to set it to Newton, this is because the allowed units, in this case, are in length unit only:
@gjedlicska Maybe it is good I tag you here to see your opinion on this.
thanks for the detailed error reports.
For the 1st issue, its our bad, I haven’t considered the case of the type
List with no generic
T type added. This is a valid python syntax (imho unfortunately), so expect a fix coming for this.
For the 2nd issue I’ll have to get back to you on this after I check how we handle things in the dotnet SDK
I’ve pushed a new release
2.11.1 fixing the typing issues. Hope this fixes your usecases.
Hey Gergő, thanks for the fixing! From my testing, the first issue is indeed solved.
I have an additional question though in this case. I ran some new testing in our package today and we encountered one new issue:
We create our own custom speckle object based on the speckle base object, so they are registered in the _type_registry. If you check the example class below, you can see the type hint for the attribute ‘control_sets_node_numbers’ is ‘Optional[Union[List[int], List[MeshNode]]]’
After we defined this speckle object and try to set the value of the attribute we got errors in the type checking. As you can see in the _type_check, the type for this attribute now is Union[List[…], List[…], None], after getting the type hint, which seems logical.
But this will give an error in _validate_type because it tries to unpack the tuple of 3 (including NoneType) into 2 variables. For us, it would occur in more places where you have more than 2 types in the Union check, could you take a look to see if there will be a solution for this issue?
thanks again for the helpful bug report, I’m looking into this. Sorry for the annoying errors, I recently had to rework our typing system quite a bit, because it had a lot of silent failures / errors, some of which you guys also noticed.
So bear with me pls, while we iron out these kinks.
Funny enough, the issue is, that the
Optional[Union[List[...], List[...]]] type hint is translated to
Union[List[...], List[...], None] in the background.
Optional[T] is a syntactic sugar type for
TIL that the nested
Union Types` are de-nested by the python type system in the background.
2.11.2 is out with a fix for this issue @He_Patrick
Haha yeah, I also learned that about the type hint today.
Thanks for the quick fix! I just tested and the issue is solved now.
And no worries I think these improvements are quite nice and important. I will let you know if I have further bug reports
Hey Gergő, I saw this post marked as solved, but I am not sure if I should make a new post or just report this bug on github, so I will put it here anyway since this is also related to type checking.
In our package, we used the function add_detachable_attrs to add a set of strings to the speckle object. This causes error during _validate_type checking because set is not yet included. Do you think it is possible to include the type checking for set type as well?
A new release
2.11.3 is coming through the pipes as I write this. One gotcha with the type checking of sets, that you should be aware of:
For performance reasons, we’re only checking the type validity of the first item in any iterable object. That, and the fact, that set order is not preserved in python, means, that type checking a set for the provided generic
T can be wonky.
It is a known limitation on our end and it will be part of a proper typing / validation discussion down the line.