Skip to content

add universaldetector #3734

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

Merged
merged 4 commits into from
Mar 12, 2020
Merged

add universaldetector #3734

merged 4 commits into from
Mar 12, 2020

Conversation

ju-sh
Copy link
Contributor

@ju-sh ju-sh commented Feb 11, 2020

Started adding files for chardet. Hopefully others will hop in to finish.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! A few remarks below.

__init__.pyi should look like this (minus the explanatory comments):

# The following line will re-export UniversalDetector like the implementation does.
# The "as ..." part is required for re-export in stubs.
from .universaldetector import UniversalDetector as UniversalDetector

# The next line will mark this stub as incomplete.
def __getattr__(name: str) -> Any: ...  # incomplete

from logging import Logger

class UniversalDetector:
if sys.version_info >= (3, 8):
from typing import TypedDict
Copy link
Collaborator

@srittau srittau Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While TypedDict is only available in typing starting in Python 3.8, you can just import it from typing_extensions for earlier Python versions:

if sys.version_info >= (3, 8):
    from typing import TypedDict
else:
    from typing_extensions import TypedDict

Type checkers support this. This way you would avoid the version checks below and make the typed dicts work with all Python versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even easier, you can also just do from typing_extensions import TypedDict so you don't need the version check.

Copy link
Contributor Author

@ju-sh ju-sh Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that both of you! Sotyping_extensions has the required info anyway?

Copy link
Contributor Author

@ju-sh ju-sh Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra Stubs of some other modules seems to be doing the version check as @srittau mentioned. As in

if sys.version_info >= (3, 8):
    from typing import Final
else:
    from typing_extensions import Final

from stdlib/3/dbm/dumb.pyi

Does this method have any advantage?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No advantage, except that whenever 3.7 support ends, a simple grep for "version_info" will flag this. But we should just grep for typing_extensions when that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean when typing_extensions stop supporting 3.7?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, when typeshed stops supporting 3.7 in a few years, we can just do from typing import Final in typeshed.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just two more suggestions.

if sys.version_info >= (3, 8):
from typing import TypedDict

class FinalResultType(TypedDict):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a typestub-only type, it should start with an underscore.

confidence: float
language: str

class IntermediateResultType(TypedDict):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I had a doubt. If I have a set of files all of which use a specific TypedDict class for typing. Should I include the TypedDict class in all these files or is there a way I can define it in one place and import it into those files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just import it from another file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like python modules? Cool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra Are there any conventions to be followed for the.pyifile where the TypedDict class is defined? The definition which can then be imported to other files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name should probably be prefixed with _ to indicate that it doesn't exist at runtime.

Copy link
Contributor Author

@ju-sh ju-sh Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got another doubt. Can you help me out?

I came across a line like this:

if sys.version_info < (3, 0):
    base_str = (str, unicode)
else:
    base_str = (bytes, str)

What should be the type of base_str? I figure it starts with something like Tuple[Type] but couldn't be sure. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra I think I found another way to keep the common class definition in one place. Just defined it inside the __init__.pyi file. Can then load with from . import <type>.

@srittau srittau merged commit b44cd29 into python:master Mar 12, 2020
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.

3 participants