Skip to content

add universaldetector - #3734

Merged
srittau merged 4 commits into
python:masterfrom
ju-sh:chardet-stub
Mar 12, 2020
Merged

srittau merged 4 commits into
python:masterfrom
ju-sh:chardet-stub

Conversation

@ju-sh

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

Copy link
Copy Markdown
Contributor

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

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread third_party/2and3/chardet/universaldetector.pyi Outdated
Comment thread third_party/2and3/chardet/universaldetector.pyi Outdated
Comment thread third_party/2and3/chardet/universaldetector.pyi Outdated

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

@srittau srittau Mar 12, 2020 •

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

@ju-sh ju-sh Mar 12, 2020 •

Copy link
Copy Markdown
Contributor Author

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?

@ju-sh ju-sh Mar 12, 2020 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread third_party/2and3/chardet/universaldetector.pyi Outdated

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@ju-sh ju-sh Mar 13, 2020 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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