Skip to content

fix download without path arguments #1591

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 6, 2020
Merged

fix download without path arguments #1591

merged 7 commits into from
Feb 6, 2020

Conversation

GabrielSimonetto
Copy link
Contributor

fix #1590

@Poolitzer
Copy link
Member

Poolitzer commented Oct 31, 2019 •
edited by Bibo-Joshi
Loading

thanks for this.

After a short discussion, we came to the conclusion that you are actually on something here. Telegram does define file_name, on which file_path is based, as optional. If you want to include this, we ask to change this though:

  • Make a proper check if file_path exists
  • If not, set the file_path to the string of the file_id
  • implement a unitest for this case

This way, the actual filename will be unique. We dont think that this is ever going to happen, but better be safe then sorry

@GabrielSimonetto
Copy link
Contributor Author

Ok, it will be done soon enough

@GabrielSimonetto
Copy link
Contributor Author

So... I have a problem.

I don't have experience with tests, I've read some of them on tests/test_file.py, but I don't understand how they work. So I would need to do a test that build a FIle with the constructor using file_id, and then download and see if an error ocurs, but on this module we don't have a valid file_id to do this, so I don't know how that could be done. Can someone point me in some direction?

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Nov 18, 2019 •
edited
Loading

Hi, sorry for the late reply. In test_file.py, a fixture file is defined (Line 29). This acts just like an ordinary Telegram File. You can pass it to your new test like

def test_new_test(self, bot, file):
    file.file_id = 5                # Just an example
    assert file.file_id == 5

Within that test you can change the attributes of file as needed for your test. If you have more questions, please don't hesitate to ask :)

Also, please merge from master so we can make sure all the other tests are passing, too.

@Bibo-Joshi Bibo-Joshi added 📋 pending-reply work status: pending-reply bug 🐛 labels Nov 24, 2019
@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Nov 24, 2019
@Bibo-Joshi
Copy link
Member

@GabrielSimonetto I took the liberty to go ahead and implement the test case ;)

@GabrielSimonetto
Copy link
Contributor Author

Thanks @Bibo-Joshi, sorry for disappearing, I've been a bit busy this month.

@Bibo-Joshi Bibo-Joshi modified the milestones: 12.5, 12.4 Feb 2, 2020
jh0ker
jh0ker approved these changes Feb 2, 2020
View reviewed changes
Copy link
Member

@jh0ker jh0ker left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Eldinnie
Copy link
Member

Eldinnie commented Feb 6, 2020

Thanks you for your contribution

@Eldinnie Eldinnie merged commit bacabbe into python-telegram-bot:master Feb 6, 2020
Eldinnie added a commit that referenced this pull request Feb 6, 2020
@Bibo-Joshi @GabrielSimonetto
@ak4zh @Eldinnie
Rename Test suite … (#1750)
84cfc6f 
* Rename Test suite

* Actually change the badge in readme

* fix download without path arguments (#1591)

* fix download without path arguments

* fix download without path arguments

* solved downloading a file without file_path or custom_path

* if no file_path, download as file_id

* Add test case

* Elaborate doc string

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

* Add default values (#1490)

* added parse_mode parameter in Updater and in Bot class to set a default parse mode for bot

* DefaultValue

* Add default parse_mode everywhere

* Renome to default_parse_mode

* Test def parse_mode for send_*, edit_message_*

* Remove duplicate code in Input* classes

* Improve handling of def_p_m for Bot methods

* Remove unneeded deletion of kwargs

* Make @log preserve signature, add bots with defaults to tests

* Fix Codacy

* Fix Travis Error

* Add default_disable_notification

* Add default_disable_web_page_preview

* Add default_disable_timeout

* add default_disable_web_page_preview for InputTextMessageContent

* add default_quote

* Try fixing test_pin_and_unpin

* Just run 3.5 tests for paranoia

* add tests for Defaults

* Revert "Just run 3.5 tests for paranoia"

This reverts commit 1baa91a.

* Tidy up parameters, move Defaults to ext

* Stage new files, because im an idiot

* remove debug prints

* change equality checks for DEFAULT_NONE

* Some last changes

* fix S&R error so that tests actually run

Co-authored-by: Ak4zh <agwl.akash@gmail.com>
Co-authored-by: Eldinnie <Eldinnie@users.noreply.github.com>

* Skip test relying on ordered dicts for 3.5 (#1752)

* Rename Test suite

* Actually change the badge in readme

Co-authored-by: Gabriel Simonetto <42247511+GabrielSimonetto@users.noreply.github.com>
Co-authored-by: Ak4zh <agwl.akash@gmail.com>
Co-authored-by: Eldinnie <Eldinnie@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jh0ker jh0ker jh0ker approved these changes

Assignees
No one assigned
Labels
🔌 bug pr description: bug 📋 pending-reply work status: pending-reply
Projects
None yet
Milestone
12.4
Development

Successfully merging this pull request may close these issues.

[BUG] Cannot Download a File if it doesnt have a file_path or if custom_path is not provided to download()
5 participants
@GabrielSimonetto @Poolitzer @Bibo-Joshi @Eldinnie @jh0ker