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

Global update for type hinting: IconStr enum and ColorStr type alias #3348

Closed
bleudev opened this issue May 24, 2024 · 4 comments
Closed

Global update for type hinting: IconStr enum and ColorStr type alias #3348

bleudev opened this issue May 24, 2024 · 4 comments

Comments

@bleudev
Copy link
Contributor

bleudev commented May 24, 2024

Please Describe The Problem To Be Solved

I want to create new enum: IconStr in types.py. IconStr enum's variables is all icons from icons.py. ColorStr = Union[HexStr, Literal['primary', 'on_primary', ...]. Also i want to use this new variables in all controls
(Optional): Suggest A Solution

Create enum IconStr with all icons from icons.py and create variable ColorStr = Union[HexStr, Literal['primary', 'on_primary', ...]

It's for my future PR :)

@ndonkoHenri
Copy link
Contributor

ndonkoHenri commented May 24, 2024

We moved away from literals in favor of enums.
Please use Enums for both instead.

You could create Colors and Icons enums (the singular form could have been better but Icon is a Control) which could be used as follows: ft.Colors.BLUE.
The types will then be Optional[Colors] and Optional[Icons].

While moving to enums we need to take care of backward compatibility for ft.colors.BLUE. For this, I suggest you dont remove the current variables in those modules now, but simply copy them into the new enum.

Let's discuss on discord if you want.

@bleudev
Copy link
Contributor Author

bleudev commented May 24, 2024

We moved away from literals in favor of enums. Please use Enums for both instead.

You could create Colors and Icons enums (the singular form could have been better but Icon is a Control) which could be used as follows: ft.Colors.BLUE. The types will then be Optional[Colors] and Optional[Icons].

While moving to enums we need to take care of backward compatibility for ft.colors.BLUE. For this, I suggest you dont remove the current variables in those modules now, but simply copy them into the new enum.

Let's discuss on discord if you want.

Thanks, but about literals with ColorStr. I don't know how make and enum string and hex color in one type hinting. Only way - use type alias with HexStr (i'll want to make new type hinting - HexStr represents hex string) and with Literal.

I don't want use this:

class Colors(Enum): ...

def a(x: Union[HexStr, Colors] # It's hard!

def a(x: Colors) # i want to use new type hinting this way

How i can make this?

@ndonkoHenri
Copy link
Contributor

I personally think we should leave colors and icons as-is. I mean, what is honestly the issue with Optional[str] ? it's short and normal - nothing fancy, no?
However, if you dont think its the case, then I suggest you could create something like OptionalString=Optional[str], then use it where necessary. I dont see the need of having HexStr, ColorStr, IconStr, etc, which are at the end of the day thesame (= Optional[str])

@FeodorFitsner
Copy link
Contributor

OK, let's leave it as is for now and settle other things. Lots of changes recently and there are even more changes towards 1.0 :)

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