Nice refactoring. If we observe slower runtime performance, we may consider revert this (but probably no)

I was also concerned about the runtime performance of this, so I ran some tests,

• best case is about the same as before since this is using a generator
• worst case when all the subclasses must be found, is less then 0.1 ms slower on my local

But I agree if this does not scale properly and causes issues we can remove it

I've reported this error and it's already fixed on the original document side

nice-to-have:
We can accept these three types and call str() when assigning to instance fields.

This is interesting to me as it's the same suggestion I had in the Java PR: slackapi/java-slack-sdk#1076 (comment)

So should we provide this kind of 'helpful' interface to developers in all our SDKs? The fact that underlying HTTP API uses strings is more coincidence and a backend implementation/runtime constraint than actually a consideration for external developers.

For what it is worth IMO I think it is a nice addition, though it does increase the complexity a little bit in the SDK side.

We could leave the type as Optional[str] since this is what is in the docs, but still call str() when assigning to instance fields.

This way our types follow the docs and a dev that passes an int or float won't get a runtime error from the slack api.

what do you think @filmaj @seratch?

That sounds good to me and was what I hinted at yesterday in our discussion with the backend team as to what we could do on the SDK side, but I would want to make sure @seratch (or, for that matter, anyone else on our team) has no issues with that approach.

My suggestion here is that, regardless of the types given by a developer, this SDK eventually sends str values to the server-side. When it comes to the use cases where a developer directly manipulates dict objects, we should not (and actually are unable to) support int/float types for sure. However, these classes are supposed to be provide better developer experience. So, enabling developers to easily use more appropriate types should make sense.

Thanks for improving this!

Nice!

This is interesting to me as it's the same suggestion I had in the Java PR: slackapi/java-slack-sdk#1076 (comment)

So should we provide this kind of 'helpful' interface to developers in all our SDKs? The fact that underlying HTTP API uses strings is more coincidence and a backend implementation/runtime constraint than actually a consideration for external developers.

For what it is worth IMO I think it is a nice addition, though it does increase the complexity a little bit in the SDK side.

I believe this is a required property

Yess it is required in the docs but the python implementation of other required fields was not done, so I followed the trend of the existing code.

What do you think?

Hmm, when I look at, for example, the Button element's text property, that is required, and is not marked as Optional, so that is why I asked / pointed it out. I am also confused 😄

In this file it seems like all action_id are optional but should be required, not sure what the standard is here @seratch what do you think?

The action_id one, I think, is a unique situation, as technically (you can try it out yourself) it is not required. You can create a block kit block with elements without action_id, and it is fine. However, you should provide it. So our public facing documentation says it is required, while in reality, it is not really required.

@filmaj @WilliamBergamin First off, is_decimal_allowed is not required actually. So, I am pretty sure that the document is wrong.

action_id

Fil's understanding is correct. For most real-world use cases, the property can be necessary but there is a rare case such as "We have only one interactive block in the app! so we'd like to skip setting action_id to distinguish the button." or something like that. So the platform does not technically require it.

Good catch 💯

0 is falsy in Python. These parts should be more explicit:

>>> if 0:
...     print("true")
... else:
...     print("false")
...
false

Also, if we accept non-string values here, the type hints for arguments need to be updated too

Ahh yess god catch 🥇 did not think about the 0 falsy

I've added type Optional[Union[int, float, str]] for initial_value, min_value, max_value