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

optimise_fonts's return value could be better typed #2

Open
shmulvad opened this issue Dec 29, 2023 · 3 comments
Open

optimise_fonts's return value could be better typed #2

shmulvad opened this issue Dec 29, 2023 · 3 comments
Assignees
Labels
pythonic Making the code more idiomatic Python

Comments

@shmulvad
Copy link

The main function optimise_fonts and the other exposed functions using it return dict[str, typing.Any]. This is inconvenient for the users of the library as they will have no autocomplete and type safety. There are two possible ways you can solve it. I would recommend 1 here, although it will break backwards compatibility. You can use 2 if you really prefer dictionaries and don't want to break the backwards compatibility.

1. Using a dataclass:

Returning a suitable dataclass instead will allow users to access the values with dot notation:

from dataclasses import dataclass, field

@dataclass
class FontResult:
    css: set[str] = field(default_factory=set)
    fonts: dict[str, str] = field(default_factory=dict)
    chars: set[str] = field(default_factory=set)
    uranges: str = ''

Then you can return an instance of this instead of your dictionary:

font_res = FontResult()
...
font_res.chars = characters  # etc
...
return font_res

2. Using TypedDict:

Return a typed dictionary. The code would be something along these lines:

from typing import TypedDict


class FontResult(TypedDict):
    css: set[str]
    fonts: dict[str, str]
    chars: set[str]
    uranges: str


def optimise_fonts(...) -> FontResult:
    font_res = {'css': set(), 'fonts': {}, 'chars': set(), 'uranges': ''}
    ...
    return font_res
@vintagedave
Copy link
Owner

return dict[str, typing.Any]. This is inconvenient for the users of the library as they will have no autocomplete and type safety

Yes! This is a great point, and worried me when I wrote it. I had no idea how to represent multiple types in a type hint; without your feedback here I likely would have gone in the direction of writing a class. That's also because there are multiple places in code where an empty/default return value is laboriously constructed, which is ugly.

But both solutions you have here are new to me: both typed dict and data classes. I'm not attached to a dict per se, it just seemed a neat / Pythonic way to collect multiple items together. Thanks for the feedback and I'll research both, but I note the recommendation for (1) and will pay attention there.

@vintagedave vintagedave self-assigned this Dec 31, 2023
@vintagedave vintagedave added the pythonic Making the code more idiomatic Python label Dec 31, 2023
@vintagedave
Copy link
Owner

Hi. I have been thinking about this for a while, and I'm still trying to figure out a solution that lets me retain the existing interface while adding another, in order not to break the library usage.

@shmulvad
Copy link
Author

shmulvad commented Dec 9, 2024

Using suggestion 2 would not change any runtime behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pythonic Making the code more idiomatic Python
Projects
None yet
Development

No branches or pull requests

2 participants