Add file extension filter by eIGato · Pull Request #2169 · python-telegram-bot/python-telegram-bot

eIGato · 2020-10-31T19:04:38Z

Resolve #2140.
Based on an abandoned PR of another contributor.
Fix #2156.

Added class 'file_ending' that filter documents which end in the extension passed by the user to Filters.document.file_ending; Added name to AUTHORS.rst.

Changed 'file_ending' to 'file_extension'; Use of fstrings on formatting instead of .format();

Added tests for some possible cases of file extensions.

eIGato · 2020-10-31T19:13:05Z

@Bibo-Joshi may you restart failed tests? I don't think they failed because of these changes. More likely, some temporary issue with test env.
Besides, no need to edit keywords in this PR. "Resolve" and "Fix" are fine too.

eIGato · 2020-10-31T19:19:42Z

Thanks.

Bibo-Joshi · 2020-10-31T19:25:18Z

NP. I'll try to review both PRs tomorrow ;)

Issue: python-telegram-bot#2140

eIGato · 2020-11-02T10:09:34Z

I think everything is fixed here.

Bibo-Joshi

As a side note: Please don't force push if not necessary. It makes reviewing harder.

Issue: python-telegram-bot#2140

eIGato · 2020-11-02T11:58:53Z

What about case sensitivity? I've made it insensitive but may be there are cases when it should not be done. For example *.Dockerfile. May be make it optional or just score on this?

eIGato · 2020-11-02T12:13:42Z

Sorry for force-pushing. I'm used to Gitlab where one can view diffs of different pushed versions. I just want to bring logically connected changes to one commit.

Bibo-Joshi · 2020-11-02T15:35:49Z

What about case sensitivity? I've made it insensitive but may be there are cases when it should not be done. For example *.Dockerfile. May be make it optional or just score on this?

Mh, good point. While intuitively I'd say lower case everything, a quick search tells me that file extension case sensitivity can depend on for operating system and also hard drive encoding or something. So I'd like to go for a flag, defaulting to lowercasing everythng. have a look at the implementation of Filters.command for that. Please add something like

Note:
  * adds the dot automatically, so *insert .jpg example here*
  * case insensitive by default, use the flag *whatever it's called*, if you need it

to the docstring

Sorry for force-pushing. I'm used to Gitlab where one can view diffs of different pushed versions. I just want to bring logically connected changes to one commit.

NP. We squash anyway before merging, so that doesn't really matter too much ;)

Bibo-Joshi

Thanks for the updates! Just two nitpicks left from my side. @Poolitzer do you want to review, too?

Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>

eIGato · 2020-11-04T12:58:26Z

Cite from your contibuting guidelines: "This gives us the flexibility to re-order arguments and more importantly to add new required arguments".
Why should it refer only to calls but not the signatures?

Bibo-Joshi · 2020-11-04T14:21:59Z

Cite from your contibuting guidelines: "This gives us the flexibility to re-order arguments and more importantly to add new required arguments".
Why should it refer only to calls but not the signatures?

Well, that style guideline applies only to code within PTB itself and not to how users should use the library. So not having , *, in the signatures allows the user to use both.
I do see the advantage of enforcing kwargs in calls, but I also see the philosophy of not handholding the user and the fact that it's a breaking change that should be done consistently throughout the lib.
If it's important to you, please go ahead and open a new issue so that it can be properly discussed, as it's beyond the scope fo this PR.

eIGato · 2020-11-05T07:57:56Z

It's a normal approach: use a good practice for the new code and fix the old code later.
Did as you wish. Any nitpicks left?

Bibo-Joshi · 2020-11-05T08:09:37Z

not from my side, but Poolitzers review is still pending. Same for #2167 btw.

Poolitzer

Left some minor comments, overall I really like this PR, thanks!

Poolitzer · 2020-11-05T19:44:27Z

+                elif case_sensitive:
+                    self.file_extension = f".{file_extension}"
+                    self.name = (
+                        f"Filters.document.file_extension({file_extension!r},"


sorry what is the !r for?

it adds quotes to the strings. Also make None distinguishable from "None". Also asked that :D

quick google didnt show that to me :(

uses repr() instead of str()

Poolitzer · 2020-11-05T20:19:25Z

oh @Bibo-Joshi can you check if you can build docs on this branch, specifically the filters page and this filter appears?

Bibo-Joshi · 2020-11-05T20:33:11Z

oh @Bibo-Joshi can you check if you can build docs on this branch, specifically the filters page and this filter appears?

It doesn't. @eIGato the docstring of file_extension needs to be copied into the docstring of document. Please do that and build the docs locally to check that it renders correctly.
thanks for the catch @Poolitzer !

eIGato · 2020-11-06T16:43:47Z

IDKY the checks fail. New code is fully covered with tests and all tests pass locally: 1716 passed, 187 skipped, 19 xfailed, 1 xpassed in 207.14 seconds.
I've tried to repush the branch to restart the checks but result did not change.

Bibo-Joshi · 2020-11-06T16:46:16Z

test official fails due to the recent API updates not yet being incorporated and codecov for some reason decided to mark a docstring as not tested, so everthing's fine.

eIGato · 2020-11-06T16:52:49Z

IDK how to fix such rendering: the renderer just understands the line feeds literally.

Bibo-Joshi · 2020-11-06T17:00:27Z

I fixed the rendering locally, but I can't push to your branch. did you disable edits by maintainers (very bottom of the panel on the right)? if not:

        file_extension: This filter filters documents by their file ending/extension.

            Note:
                * This Filter only filters by the file ending/extension of the document,
                  it doesn't check the validity of document.
                * The user can manipulate the file extension of a document and
                  send media with wrong types that don't fit to this handler.
                * Case insensitive by default,
                  you may change this with the flag ``case_sensitive=True``.
                * Extension should be passed without leading dot
                  unless it's a part of the extension.
                * Pass :obj:`None` to filter files with no extension,
                  i.e. without a dot in the filename.

            Example:
                * ``Filters.document.file_extension("jpg")`` filters files with extension
                  ``".jpg"``.
                * ``Filters.document.file_extension(".jpg")`` filters files with extension
                  ``"..jpg"``.
                * ``Filters.document.file_extension("Dockerfile", case_sensitive=True)``
                  filters files with extension ``".Dockerfile"`` minding the case.
                * ``Filters.document.file_extension(None)`` filters files without a dot in the
                  filename.

Poolitzer

LGTM with the rendering fixes bibo suggested

mlemosf added 3 commits October 31, 2020 21:54

Added: Filter for file ending/extension

0cf0b43

Added class 'file_ending' that filter documents which end in the extension passed by the user to Filters.document.file_ending; Added name to AUTHORS.rst.

Fixed sugested corrections

4bdc2ae

Changed 'file_ending' to 'file_extension'; Use of fstrings on formatting instead of .format();

Added unit tests for class test_ending

6b4d0fa

Added tests for some possible cases of file extensions.

Bibo-Joshi requested changes Oct 31, 2020

View reviewed changes

Comment thread telegram/ext/filters.py Outdated

Comment thread telegram/ext/filters.py Outdated

Comment thread tests/test_filters.py Outdated

Comment thread telegram/ext/filters.py Outdated

eIGato added 2 commits November 2, 2020 11:05

Mind the dot in file extensions

ef7b41f

Issue: python-telegram-bot#2140

Add myself to contibutors list

e54c23e

eIGato force-pushed the file-ending-filter branch from 1ee4667 to e54c23e Compare November 2, 2020 08:08

Bibo-Joshi requested changes Nov 2, 2020

View reviewed changes

Comment thread telegram/ext/filters.py Outdated

Comment thread telegram/ext/filters.py Outdated

Comment thread telegram/ext/filters.py Outdated

eIGato added 2 commits November 2, 2020 14:48

Claim that dot should not be passed to filter

d758320

Issue: python-telegram-bot#2140

Make extension filter case insensitive

e7fbe09

Issue: python-telegram-bot#2140

eIGato added 3 commits November 3, 2020 00:39

Make case sensitivity optional

57da616

Remove None from filter name

27cb0d5

Make None filter check there is no dot in filename

2b83d66

Bibo-Joshi requested changes Nov 3, 2020

View reviewed changes

Comment thread telegram/ext/filters.py Outdated

Comment thread telegram/ext/filters.py Outdated

eIGato and others added 2 commits November 4, 2020 12:59

Update documentation literacy

63503ef

Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>

Conform filter signature

f5e28f0

Bibo-Joshi requested a review from Poolitzer November 5, 2020 08:09

eIGato requested a review from Bibo-Joshi November 5, 2020 08:25

Bibo-Joshi removed their request for review November 5, 2020 09:44

Poolitzer requested changes Nov 5, 2020

View reviewed changes

eIGato added 2 commits November 6, 2020 11:02

Add None example to the filter

500f54b

Copy-paste the documentation

dc000d0

eIGato force-pushed the file-ending-filter branch from 1692ee3 to dc000d0 Compare November 6, 2020 15:53

Poolitzer reviewed Nov 6, 2020

View reviewed changes

Fix docs indentation

18ce77a

Bibo-Joshi merged commit ac449de into python-telegram-bot:master Nov 6, 2020

github-actions Bot locked and limited conversation to collaborators Nov 7, 2020

Bibo-Joshi added the hacktoberfest-accepted other: hacktoberfest-accepted label Nov 7, 2020

Conversation

eIGato commented Oct 31, 2020

Uh oh!

eIGato commented Oct 31, 2020

Uh oh!

eIGato commented Oct 31, 2020

Uh oh!

Bibo-Joshi commented Oct 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eIGato commented Nov 2, 2020

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eIGato commented Nov 2, 2020

Uh oh!

eIGato commented Nov 2, 2020

Uh oh!

Bibo-Joshi commented Nov 2, 2020

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eIGato commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bibo-Joshi commented Nov 4, 2020

Uh oh!

eIGato commented Nov 5, 2020

Uh oh!

Bibo-Joshi commented Nov 5, 2020

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Poolitzer Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Poolitzer Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Poolitzer commented Nov 5, 2020

Uh oh!

Bibo-Joshi commented Nov 5, 2020

Uh oh!

eIGato commented Nov 6, 2020

Uh oh!

Bibo-Joshi commented Nov 6, 2020

Uh oh!

eIGato commented Nov 6, 2020

Uh oh!

Bibo-Joshi commented Nov 6, 2020

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

eIGato commented Nov 4, 2020 •

edited

Loading