8000 Add typing info to plistlib by madig · Pull Request #2061 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add typing info to plistlib #2061

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Sep 21, 2020
Merged

Add typing info to plistlib #2061

merged 21 commits into from
Sep 21, 2020

Conversation

madig
Copy link
Collaborator
@madig madig commented Sep 14, 2020 •
edited
Loading

Wherein I spend my life adding typing information to the plistlib module and use Pylance in strict checking mode. Maybe I'll get somewhere.

First boss: microsoft/pyright#988

I wish I could just kill Data.

The module is half red because the etree module could do with some typing information, too.

@chrissimpkins hi there

@justvanrossum
Copy link
Collaborator

That singledispatch register section is pretty disgusting. I find this a high price to pay.

@madig
Copy link
Collaborator Author
madig commented Sep 15, 2020 via email

anthrotype
anthrotype reviewed Sep 15, 2020
View reviewed changes
@anthrotype
Copy link
Member

I wish I could just kill Data.

I think we can, it was there for backward compatibility with old python2 plistlib API

@anthrotype
Copy link
Member

The module is half red because the etree module could do with some typing information, too.

could these https://github.com/JelleZijlstra/lxml-stubs help maybe?

anthrotype
anthrotype reviewed Sep 15, 2020
View reviewed changes
Lib/fontTools/misc/plistlib.py Outdated
import sys
import re
from typing import Any, Callable, Dict, List, NoReturn, Optional, Sequence, Tuple, Union, overload
from typing import Any, Callable, Dict, List, Mapping, NoReturn, Optional, Sequence, Tuple, Type, TypeVar, Union, overload, IO
Copy link
Member

Choose a reason for hiding this comment

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

(perhaps the PR is unfinished but) I see two almost duplicated from typing import * lines above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll clean up later. Still fudging about between random support requests.

@madig
Copy link
Collaborator Author
madig commented Sep 15, 2020 •
edited
Loading

Btw, do we really need dict_type in load*? I see tests that use OrderedDictionary but we can just make that the unchangeable, default type (since dict is only guaranteed to be ordered in Python 3.7+)?

@anthrotype
Copy link
Member

dict_type is part of the built-in plistlib API

https://github.com/python/cpython/blob/a0da90720d5330c53b8083272374ede1c7a1e33a/Lib/plistlib.py#L973

I wouldn't like to lose it because of too strict/lazy typecheckers

@madig
Copy link
Collaborator Author
madig commented Sep 15, 2020 •
edited
Loading

Ugh. The fromtree and load* functions may not only return a dictionary, but any object, e.g. when the plist file just contains <real>1.0</real>. So the typeshed typing stub is wrong?!

python/typeshed#4542

anthrotype
anthrotype reviewed Sep 16, 2020
View reviewed changes
Copy link
Member
@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

overall, lgtm (with comments)

Should you not use "PlistEncodable" instead of Any where you use Mapping[str, Any] or Sequence[Any]?

@anthrotype
Copy link
Member

Mypy does not support recursive type aliases

ah :(

@madig madig marked this pull request as ready for review September 16, 2020 19:20
@madig
Copy link
Collaborator Author
madig commented Sep 16, 2020 •
edited
Loading

Ok, I think this is it for now. @anthrotype so, I put in four type-ignores and three of them are for etree confusion. I'm not exactly sure yet what's causing it, but I'd look at it next. The last one is masking a diff between the return containers of dict.items() and sorted(dict.items())...

anthrotype
anthrotype approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member
@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

overall LGTM.

pls fix ImportError in ufoLib.plistlib (see Travis CI error)

madig and others added 3 commits September 21, 2020 16:53
@madig @anthrotype
Update Lib/fontTools/misc/plistlib/__init__.py
4efc06e 
Co-authored-by: Cosimo Lupo <cosimo@anthrotype.com>
@madig @anthrotype
Update Lib/fontTools/misc/plistlib/__init__.py
a1df979 
Co-authored-by: Cosimo Lupo <cosimo@anthrotype.com>
@madig
Implement suggestions
0742a9b
@anthrotype anthrotype merged commit b913fac into master Sep 21, 2020
@anthrotype anthrotype deleted the plistlib-typing branch September 21, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthrotype anthrotype anthrotype approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madig @justvanrossum @anthrotype
0