Skip to content

Fix Filters documentation #570

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

Closed
wants to merge 4 commits into from

Conversation

revolter
Copy link
Contributor

No description provided.

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

Thank you :) Do you want to add yourself to AUTHORS.rst before I merge?

@jh0ker jh0ker added ⚙️ documentation affected functionality: documentation 📋 pending-merge work status: pending-merge 📋 pending-reply work status: pending-reply labels Apr 25, 2017
@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

Actually, this fails on the flake8 pre-commit hook. To quote @bomjacob:

I think that's the original reason I had for not including those MessageEntity in the first place, since the docs can't show it properly if you break it into two lines either...

If you can solve this without exceeding the maximum line length of 99 characters while still producing a nice-looking doc page, that would be great.

@jh0ker jh0ker removed the 📋 pending-merge work status: pending-merge label Apr 25, 2017
@Eldinnie
Copy link
Member

Eldinnie commented Apr 25, 2017 via email

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

@Eldinnie That sounds reasonable to me

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

To make sure it doesn't show up in the rendered HTML page, it should also be prepended with .. (rst comment). Not sure if this works inline though.

@revolter
Copy link
Contributor Author

Yeah, that's some pretty hard line to break. Are you up to add the comment yourself? As it allows edits from maintainers.

@Eldinnie
Copy link
Member

yes so to be complete: append the line that's too long with .. # noqa: E501 to make it pass flake8's test but not show up in the docs.

Ignore long line for flake
@revolter
Copy link
Contributor Author

I don't get what's wrong this time, coveralls is showing all errors, not only those added by this PR :-?

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

You can ignore the coveralls check for changes like this, it sometimes fucks up

@revolter
Copy link
Contributor Author

Done

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

image
Sadly, it does not seem to work :/

@revolter
Copy link
Contributor Author

Where is that screenshot from?

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

I cloned your repo/branch and built the documentation

@revolter
Copy link
Contributor Author

With Sphinx?

@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

Yes. python setup.py build_sphinx

@Eldinnie
Copy link
Member

@jh0ker autodoc maybe ignores it... dunno how to make it ignored in the docs

@revolter
Copy link
Contributor Author

Would this:

        >>> (
        >>>     Filters.text & (
        >>>         Filters.entity(MessageEntity.URL) | Filters.entity(MessageEntity.TEXT_LINK)
        >>>     )
        >>> )

be acceptable? I think it also helps the readability by separating those 3 ending parenthesis.

@jh0ker jh0ker removed the 📋 pending-reply work status: pending-reply label Apr 25, 2017
@jh0ker
Copy link
Member

jh0ker commented Apr 25, 2017

I think that is a reasonable compromise. Please use ... instead of >>> to denote the continued expression over multiple lines

@revolter
Copy link
Contributor Author

I didn't know the exact syntax, I added now the commit.

@revolter
Copy link
Contributor Author

Why are there so many unhashable type: 'list' errors in Travis?

@Eldinnie
Copy link
Member

@revolter see #579

@Eldinnie
Copy link
Member

I overhauled filters docs in #728 so closing this one.

@Eldinnie Eldinnie closed this Jul 19, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚙️ documentation affected functionality: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants