Skip to content

Conversation

@eIGato
Copy link
Contributor

@eIGato eIGato commented Oct 31, 2020

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
Copy link
Contributor Author

eIGato commented Oct 31, 2020

@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
Copy link
Contributor Author

eIGato commented Oct 31, 2020

Thanks.

@Bibo-Joshi
Copy link
Member

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

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

eIGato commented Nov 2, 2020

I think everything is fixed here.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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.

@eIGato
Copy link
Contributor Author

eIGato commented Nov 2, 2020

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
Copy link
Contributor Author

eIGato commented Nov 2, 2020

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
Copy link
Member

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 ;)

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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?

eIGato and others added 2 commits November 4, 2020 12:59
Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
@eIGato
Copy link
Contributor Author

eIGato commented Nov 4, 2020

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
Copy link
Member

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
Copy link
Contributor Author

eIGato commented Nov 5, 2020

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
Copy link
Member

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

@Bibo-Joshi Bibo-Joshi requested a review from Poolitzer November 5, 2020 08:09
@eIGato eIGato requested a review from Bibo-Joshi November 5, 2020 08:25
@Bibo-Joshi Bibo-Joshi removed their request for review November 5, 2020 09:44
Copy link
Member

@Poolitzer Poolitzer left a 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},"
Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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 :(

Copy link
Member

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()

@Poolitzer
Copy link
Member

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

@Bibo-Joshi
Copy link
Member

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 eIGato force-pushed the file-ending-filter branch from 1692ee3 to dc000d0 Compare November 6, 2020 15:53
@eIGato
Copy link
Contributor Author

eIGato commented Nov 6, 2020

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
Copy link
Member

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
Copy link
Contributor Author

eIGato commented Nov 6, 2020

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

@Bibo-Joshi
Copy link
Member

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.

Copy link
Member

@Poolitzer Poolitzer left a 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

@Bibo-Joshi Bibo-Joshi merged commit ac449de into python-telegram-bot:master Nov 6, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2020
@Bibo-Joshi Bibo-Joshi added the hacktoberfest-accepted other: hacktoberfest-accepted label Nov 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

hacktoberfest-accepted other: hacktoberfest-accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] introduce a file ending filter

4 participants