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

Add stubs for bleach #2709

Merged
merged 3 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@srittau
Copy link
Collaborator

commented Dec 21, 2018

Permission in mozilla/bleach#424

@JelleZijlstra
Copy link
Collaborator

left a comment

Left a few comments around Python 2 support.


DEFAULT_CALLBACKS: List[_Callback]

TLDS: List[str]

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Dec 21, 2018

Collaborator

Text actually (the file has unicode_literals). I think most of the strs below should also be Text.

PHOTO_RE: Pattern[str]
EMAIL_RE: Pattern[str]

class Linker:

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Dec 21, 2018

Collaborator

Should inherit from object.

However, I just noticed that the stubs are in third_party/3. Why is that? The library looks like it's 2/3-compatible.

@@ -0,0 +1,34 @@
from typing import List, Dict, Any, Optional, Type, Pattern, Union, Callable, Container

ALLOWED_TAGS: List[str]

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Dec 21, 2018

Collaborator

Also Text

strip_comments: bool = ...,
filters: Optional[List[Type[_Filter]]] = ...,
) -> None: ...
def clean(self, text: str) -> str: ...

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Dec 21, 2018

Collaborator

Returns Text.

@srittau

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 21, 2018

I have updated the stubs now to support Python 2. But I wonder whether we should require contributors to provide Python 2 compatible stubs when they submit new stubs. (Of course existing stubs that support Python 2 should continue to do so.) Making stubs Python 2 compatible requires quite a bit of additional work and it's quite possible that a Python 2 version is not that useful anyway, considering Python 2's end of life date.

Personally I suggest, we accept Python 3-only stubs (and also Python 2-only stubs), even if the library supports both versions. Support for the other Python version can always be added later by an interested party.

@srittau

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 10, 2019

Gentle ping @JelleZijlstra

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Sorry for dropping this!

@JelleZijlstra JelleZijlstra merged commit 6f00053 into python:master Mar 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@srittau srittau deleted the srittau:bleach branch Mar 11, 2019

@Deimos

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Just installed the new version of mypy and I'm getting some errors on my codebase now from these stubs. Specifically, I'm using bleach's Cleaner class, which has its __init__ stub as:

class Cleaner(object):
    def __init__(
        self,
        tags: List[Text] = ...,
        attributes: Any = ...,
        styles: List[Text] = ...,
        protocols: List[Text] = ...,
        strip: bool = ...,
        strip_comments: bool = ...,
        filters: Optional[List[Type[_Filter]]] = ...,
    ) -> None: ...

I'm passing tuples for both tags= and protocols=, which works fine but doesn't satisfy these stubs. Maybe the stubs should use Sequence instead of explicitly defining List (both here and other places where that's relevant)?

Also, I'm passing a list containing a partial object for filters=. Using a partial object this way is shown in the documentation as a way to both sanitize and linkify in the same step. However, the current stubs throw the following error for doing this:

error: List item 0 has incompatible type "partial[LinkifyFilter]"; expected "Type[Any]"

@srittau would you be able to fix these issues? I could probably also submit a pull request myself to adjust them, if you'd like (though I've never contributed any stubs before so I'm not very familiar with any of what goes into it).

ALLOWED_PROTOCOLS as ALLOWED_PROTOCOLS,
ALLOWED_STYLES as ALLOWED_STYLES,
ALLOWED_TAGS as ALLOWED_TAGS,
Cleaner as Clear,

This comment has been minimized.

Copy link
@Deimos

Deimos Apr 15, 2019

Contributor

Also, I think this was a typo - the name shouldn't be getting changed, right?

Deimos added a commit to spectria/tildes that referenced this pull request Apr 15, 2019

Pin mypy version to 0.670 temporarily
mypy version 0.700 includes some stubs for the bleach library, but
they're not correct and cause errors when run on the Tildes codebase.
I've left a comment on the typeshed repo here, so hopefully it will be
resolved before long:
python/typeshed#2709 (comment)

I'll pin mypy's version to the previous version of 0.670 for now to
avoid this, but if the stubs aren't fixed for a while I may need to do
it differently.
@srittau

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

We welcome pull requests and we can help if you are stuck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.