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

Improve regex filter docstring and avoid compiling compiled regex #1314

Conversation

Ambro17
Copy link
Contributor

@Ambro17 Ambro17 commented Dec 17, 2018

This PR improves the docstring of Filters.regex. Changed format to comply with the rest of the file, and specify that pattern can be either a string or a pattern object. Also avoids recompiling regex on regex instantiation.
Add test case and define Filter attribute so sphinx can show the proper message instead of the placeholder text

Copy link
Member

@jsmnbom jsmnbom left a comment

Choose a reason for hiding this comment

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

All in all this looks like a good change! :D
I have a tiny nitpicky change I'd like you to make if you could, but other than that it LGTM.

@@ -173,25 +173,32 @@ def filter(self, message):
command = _Command()
""":obj:`Filter`: Messages starting with ``/``."""

class regex(BaseFilter):
class _Regex(BaseFilter):
Copy link
Member

Choose a reason for hiding this comment

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

We actually have several filters that don't follow this pattern further down in the file.
As far as I can see we've set done it such that:

  • All filters that don't need an input, name the class _Filter and init it as filter = _Filter()
  • All filters that do need input, break PEP8 style and name the class filter - this also improves docs if I recall correctly.

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 are right, i only saw the first filters so i asumed it was a distraction. Effectively, filters that expect an argument don't have the underscore prefix.
I'll change it today if i have time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, i'm not very familiar with sphinx so i would like to know if there's a way to build the docs to check the comment with the filter description (L212) is rendered as expected

Copy link
Member

Choose a reason for hiding this comment

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

Hey @Ambro17,
you can find a installation guide to sphinx here. Maybe you need to install spinx.ext.napoleon, sphinx_rtd_theme or other dependencies of conf.py.
You can build the docs by running make html in the docs directory. The HTML pages will be saved to docs/build/html, with index.html being the start page. The output of the make will also tell you, if dependencies are missing.
Hope this helps :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bibo-Joshi Thanks! I managed to get it to look as i like. Not sure if you agree, here's a screenshot of how it looks now
image

Copy link
Member

@Bibo-Joshi Bibo-Joshi Jan 6, 2019

Choose a reason for hiding this comment

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

Glad to help, @Ambro17!
A few minor suggestions:

  • Note: is a keywoard in sphinx, so you can easily create those fancy Note-Boxes (like e.g. here).
  • one can insert a code block with a double double-dot ::, which can help to make the text more readable. However, this surely is a question of taste.
  • I think the filter method should not get a docstring. It seems to me that the type of docstring you inserted was only added to filters that don't need an input and are initialized as filter = _Filter(), e.g. here
    class _Regex(BaseFilter):
        """
        Filters updates by searching for an occurrence of ``pattern`` in the message text.
        The ``re.search`` function is used to determine whether an update should be filtered.
        
        Refer to the documentation of the ``re`` module for more information.
            
        Note:
            Does not allow passing groups or a groupdict like the ``RegexHandler`` yet,
            but this will probably be implemented in a future update, gradually phasing out the
            RegexHandler (See `Github Issue
            <https://github.com/python-telegram-bot/python-telegram-bot/issues/835/>`_).
        
        Examples:
            Use::
            
                MessageHandler(Filters.regex(r'help'), callback)
            
            to capture all messages that contain the word help. You can also use::

                MessageHandler(Filters.regex(re.compile(r'help', re.IGNORECASE), callback)
                
             if you want your pattern to be case insensitive. This approach is recommended
             if you need to specify flags on your pattern.
            
        Args:
            pattern (:obj:`str` | :obj:`Pattern`): The regex pattern.
        """

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Added the proper spacing after Note: so it's rendedered nicely.
  2. I checked this option and it seems cool but it sets a green background for the code. As all the other filters have the `` notation, i would prefer to mantain the same style over all filters, but tell me what you think
  3. I agree that filters that don't receive arguments don't have the filter right now, but i'm not sure that was on purpose. As it currently renders a default message which is not correct or at least is confusing.
    image
    The overriding of the filter method is done under the hood, users should not override the filter method, only build the filter with the desired regex.
    Personally i think that adding the filter behaviour for that case makes more sense than rendering the BaseFilter default message.
    image

@jsmnbom jsmnbom added the 📋 pending-reply work status: pending-reply label Jan 4, 2019
@jsmnbom
Copy link
Member

jsmnbom commented Feb 8, 2019

Tested locally and seems good to me (CI is a bit broken right now)
Merging primarily to avoid conflicts with improving the regex filter for V12.
Thanks a lot for the PR :)

@jsmnbom jsmnbom merged commit 487bce1 into python-telegram-bot:master Feb 8, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📋 pending-reply work status: pending-reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants