-
Notifications
You must be signed in to change notification settings - Fork 6k
Add file extension filter #2169
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 file extension filter #2169
Conversation
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.
|
@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. |
|
Thanks. |
|
NP. I'll try to review both PRs tomorrow ;) |
1ee4667 to
e54c23e
Compare
|
I think everything is fixed here. |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note: Please don't force push if not necessary. It makes reviewing harder.
|
What about case sensitivity? I've made it insensitive but may be there are cases when it should not be done. For example |
|
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. |
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 to the docstring
NP. We squash anyway before merging, so that doesn't really matter too much ;) |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
|
Cite from your contibuting guidelines: "This gives us the flexibility to re-order arguments and more importantly to add new required arguments". |
Well, that style guideline applies only to code within PTB itself and not to how users should use the library. So not having |
|
It's a normal approach: use a good practice for the new code and fix the old code later. |
|
not from my side, but Poolitzers review is still pending. Same for #2167 btw. |
Poolitzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments, overall I really like this PR, thanks!
| elif case_sensitive: | ||
| self.file_extension = f".{file_extension}" | ||
| self.name = ( | ||
| f"Filters.document.file_extension({file_extension!r}," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry what is the !r for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it adds quotes to the strings. Also make None distinguishable from "None". Also asked that :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick google didnt show that to me :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses repr() instead of str()
|
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 |
1692ee3 to
dc000d0
Compare
|
IDKY the checks fail. New code is fully covered with tests and all tests pass locally: |
|
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. |
|
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the rendering fixes bibo suggested

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