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

Update emoji (3rd party) library #7884

Merged
merged 14 commits into from May 22, 2022
Merged

Conversation

RensOliemans
Copy link
Contributor

@RensOliemans RensOliemans commented May 19, 2022

I'm new to typeshed, hopefully this is OK.

RensOliemans and others added 5 commits May 19, 2022
Not sure if this is the correct method, but it seemed the most logical thing.
Again, not sure if this is how it works. Perhaps the source code has
been updated [1], and separate language files don't make sense any more?

[1]: (current source code):
https://github.com/carpedm20/emoji/tree/master/emoji/unicode_codes
@github-actions

This comment has been minimized.

Apparently we have to use collections.abc.Callable instead of typing.Callable, is that right?
@github-actions

This comment has been minimized.

From `emoji/core.py`, "handle_version can be either a string or a
callable; If it is a callable, it's passed the unicode emoji and the
data dict from emoji.EMOJI_DATA [which is dict[str, dict[str, str]]] and
must return a replacement string to be used." [1]

[1]: https://github.com/carpedm20/emoji/blob/master/emoji/core.py#L74
@github-actions

This comment has been minimized.

@RensOliemans
Copy link
Contributor Author

@RensOliemans RensOliemans commented May 19, 2022

Wow, checking the stubs with the actual third party code is awesome! I have a question on how I can solve this error.

The error given is:

error: emoji.emoji_lis is inconsistent, runtime argument "language" has a default value of type builtins.type, which is incompatible with stub argument type builtins.str

Which makes sense, since they changed the language argument to default to _DeprecatedParameter which is a custom class of theirs. See here for their code. Concretely, how can I refer to their custom class as a type?

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 19, 2022

I'm new to typeshed, hopefully this is OK.

Welcome to the project! PRs improving our stubs are very welcome here :)

Concretely, how can I refer to their custom class as a type?

To tackle the stubtest error for demojize, which has a similar error for the use_aliases parameter, you could do it like this:

class _DeprecatedParameter: ...

def demojize(
    string: str,
    use_aliases: bool | type[_DeprecatedParameter] = ...,
    delimiters: tuple[str, str] = ...,
    language: str = ...,
    version: float | int | None = ...,
    handle_version: str | Callable[[str, dict[str, str]], str] | None = ...,
) -> str: ...

Since the use_aliases parameter here is, well, deprecated, we could also consider just removing it from the stub (there are ways of getting stubtest to shut up about small discrepancies compared to the runtime). But it's usually best to stick as closely to the runtime as possible, and I think that's what I'd prefer here.

@github-actions

This comment has been minimized.

@RensOliemans
Copy link
Contributor Author

@RensOliemans RensOliemans commented May 20, 2022

Welcome to the project! PRs improving our stubs are very welcome here :)

Thanks!

Since the use_aliases parameter here is, well, deprecated, we could also consider just removing it from the stub (there are ways of getting stubtest to shut up about small discrepancies compared to the runtime). But it's usually best to stick as closely to the runtime as possible, and I think that's what I'd prefer here.

Alright makes sense, thanks for the review and tip!

srittau
srittau previously requested changes May 20, 2022
Copy link
Collaborator

@srittau srittau left a comment

Thanks, just a few nits below.

stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Thanks, this is great! Just a few small points to fix:

stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
stubs/emoji/emoji/core.pyi Outdated Show resolved Hide resolved
@srittau srittau dismissed their stale review May 20, 2022

@AlexWaygood mentioned everything I did and more, no need to have a duplicate review.

According to PEP 484, type checkers should treat int as an implicit
subtype of float.
@RensOliemans
Copy link
Contributor Author

@RensOliemans RensOliemans commented May 22, 2022

Thanks for the useful review tips! I have added TypedDict for the emoji_lis and emoji_list return types.

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented May 22, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Stubtest's happy, primer's happy, I'm happy. Thanks again!

@AlexWaygood AlexWaygood merged commit 6e18441 into python:master May 22, 2022
49 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants