Feat: Rewrite File.download into download_to_drive and download_to_object by Poolitzer · Pull Request #3223 · python-telegram-bot/python-telegram-bot

Poolitzer · 2022-09-06T13:53:25Z

TODO after merge: Update wiki and transition guide/script

Checklist for PRs

Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
Created new or adapted existing unit tests
Documented code changes according to the CSI standard
Added myself alphabetically to AUTHORS.rst (optional)
Added new classes & modules to the docs and all suitable __all__ s

Poolitzer · 2022-09-12T18:38:03Z

I dont get codecov path, test_download_file_obj covers that, no?

Lines 157 to 167 in 27a0fca

    
           async def test_download_file_obj(self, monkeypatch, file): 
        
               async def test(*args, **kwargs): 
        
                   return self.file_content 
        
               monkeypatch.setattr(file.get_bot().request, "retrieve", test) 
        
               with TemporaryFile() as custom_fobj: 
        
                   out_fobj = await file.download_to_object(out=custom_fobj) 
        
                   assert out_fobj is custom_fobj 
        
                   out_fobj.seek(0) 
        
                   assert out_fobj.read() == self.file_content

Edit: Doesn't complain anymore. I think it has seen its error :D

Bibo-Joshi

Thanks for the PR! I left some comments, some of which are not actually due to your changes :D

IISC Codecov mainly complains about decrypting files with self._credentials not being covered.

Regarding naming, I'd like to also throw download_to_file (as alternative to download_to_drive) and download_to_memory (as alternative to download_to_obj) into the race.

telegram/_files/file.py

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

telegram/_files/file.py

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

Poolitzer · 2022-10-06T16:21:51Z

There are open questions I have now, unordered as I think of them.

Did it make sense that I did this whole encrypted file way? It feels to me like the natural way to test this, but maybe there is a better way?
- If that makes sense to use, shouldn't we use this in different tests?
Should we rename the hash param in decrypt, it shadows a builtin function.
Why does EncryptedPassportElement take credentials argument?
Does it make sense not to apply the decryption on local files? Because thats what currently happens...

Bibo-Joshi · 2022-10-24T20:26:06Z

So I finally got around to review this again and I apologize for taking so long.
The new test look good to me, I understood what they do while reading them. However, it would be nice to have some comments that explain how you created the image_de/encrypted and the input for FileCredentials. That would have the benefits of a) being able to reproduce things and b) have even better understanding of what's going on.

Did it make sense that I did this whole encrypted file way? It feels to me like the natural way to test this, but maybe there is a better way?

LGTM.

If that makes sense to use, shouldn't we use this in different tests?

That would mean tuching passport tests. grrrrrrr. I'm okay with only touching them if absolutely necessary xD

Should we rename the hash param in decrypt, it shadows a builtin function.

Okay with me, can be a separate PR though.

Why does EncryptedPassportElement take credentials argument?

No clue, goes back to #1168. IISC removing should be fine, maybe in the same PR as the hash param

Does it make sense not to apply the decryption on local files? Because thats what currently happens...

It does not, actually 🤔 Even for local files, they should be encrypted, I guess. Can you fix that + add tests?

Also improved docs for the tests

harshil21

nice work, didn't look at the tests too closely:

telegram/_files/file.py

Bibo-Joshi

Thanks for the updates & the improvements for local encrypted files!

Bibo-Joshi · 2022-10-31T15:06:10Z

examples/conversationbot.py

This may have to change to (20, 0, 0, "beta", 0) … suggestions for a good place to keep track of that? 😬

Maybe in the release page? But it's hard to say when a change breaks an example post those changes

I'm mainly talking about our plans to make the next release a beta release. Only if we get another API update before #3249 is mergable, the next release would be calld 20.0a5 … Mind you, this check would still be correct, since (20, 0, 0, "alpha", 5) < (20, 0, 0, "beta", 0), but it would probably still confuse users xD I'll add a note o the "how to release page" as reminder :)

telegram/_files/file.py

Also rename to_drive to to_memory

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Bibo-Joshi

precommit and doc-build are failing 🤔
otherwise LGTM 🙂

harshil21

LGTM

Poolitzer added 2 commits September 6, 2022 15:52

Feat: Merge out and custom_path into out

5b9458d

Fix: Make deepsource happy

8e13681

harshil21 added this to the v20.0a5 milestone Sep 9, 2022

harshil21 added enhancement 🛠 breaking change type: breaking 🛠 refactor change type: refactor and removed enhancement labels Sep 9, 2022

Poolitzer added 3 commits September 12, 2022 20:03

Feat: Rewrite to two functions

27a0fca

Fix: Missed passing to retrieve call

794a003

Fix: Make sphinx happy

2fcbf66

Poolitzer changed the title ~~Feat: File.download, merge out and custom_path into out~~ Feat: Rewrite File.download into download_to_drive and download_to_object Sep 12, 2022

Fix: Make pre-commit happy

c30a3b8

Poolitzer added 2 commits September 12, 2022 20:49

Fix: one old function

acb4979

Fix: stupid test

5abe2cb

Bibo-Joshi requested changes Sep 13, 2022

View reviewed changes

Poolitzer and others added 3 commits September 13, 2022 13:02

Apply suggestions from code review

44b791c

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Fix: small review improvements

f530099

Fix: improve tests

f48c523

Lauraki approved these changes Sep 13, 2022

View reviewed changes

harshil21 requested changes Sep 16, 2022

View reviewed changes

Poolitzer and others added 5 commits September 19, 2022 21:00

Apply suggestions from harshil

108186b

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

Fix: test awaited for non existing return

f601972

Fix: sphinx complaining

bd50405

Merge branch 'master' into rewrite_file_params

58adde2

Fix: Apply review suggestions

a7a4a4c

harshil21 added the ℹ️ needs-wiki-update information: needs-wiki-update label Sep 24, 2022

Poolitzer added 2 commits October 6, 2022 18:08

Improvement: Add en-/decryption download test

1732e7b

Improvement: Move decrypt call to protected method

f1b8bef

Poolitzer added 4 commits October 8, 2022 11:02

merging?

f1bf448

Fix: Extra documentation line sneaked in

0b53697

Fix: Remove double warning

783da37

Fix: Broken test setup

86eeb11

Feat: Add encryption to local files + bytearrays

3dfbed6

Also improved docs for the tests

harshil21 reviewed Oct 31, 2022

View reviewed changes

telegram/_files/file.py Show resolved Hide resolved

Bibo-Joshi reviewed Oct 31, 2022

View reviewed changes

Poolitzer and others added 2 commits October 31, 2022 19:07

Fix: Declutter code

0146877

Also rename to_drive to to_memory

Apply suggestions from Bibo-Joshi

8dfd9d9

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Bibo-Joshi approved these changes Nov 1, 2022

View reviewed changes

Poolitzer added 2 commits November 1, 2022 12:19

Fix: improve doc string

41aa362

Fix: update version check in examples

7279afe

harshil21 approved these changes Nov 1, 2022

View reviewed changes

Bibo-Joshi merged commit 636654c into master Nov 2, 2022

Bibo-Joshi deleted the rewrite_file_params branch November 2, 2022 07:28

github-actions bot locked and limited conversation to collaborators Nov 9, 2022

Conversation

Poolitzer commented Sep 6, 2022 • edited by harshil21 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for PRs

Uh oh!

Poolitzer commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Poolitzer commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bibo-Joshi commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Poolitzer Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Poolitzer commented Sep 6, 2022 •

edited by harshil21

Loading

Poolitzer commented Sep 12, 2022 •

edited

Loading

Poolitzer commented Oct 6, 2022 •

edited

Loading

Bibo-Joshi commented Oct 24, 2022 •

edited

Loading