Skip to content
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 max retries for downloader #45

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Conversation

yz3440
Copy link
Contributor

@yz3440 yz3440 commented Jun 22, 2024

Having a while True in the library is scary. I have had infinite loops when I have a special bad panorama ID.

Normal non-existent ID points to an url that's requestable, but gives back bad data, which causes the program to error out on the image parsing exception. There are also a few special bad ID that straight cannot be requested, and the program will keep trying til the end of the world. I don't have example IDs in hand at the moment, but please trust me on this. My guess is that these ID exists but are unpublished by Google due to whatever reason.

I'm suggesting to enforce a default max_retries of 6 for the download functions, and also make it to be user configurable.

@yz3440
Copy link
Contributor Author

yz3440 commented Jun 22, 2024

@robolyst kindly asking for your attention again, have a great weekend

Copy link
Owner

@robolyst robolyst left a comment

Choose a reason for hiding this comment

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

Nice!

I see some of the function arguments look like:

max_retries: int = DEFAULT_MAX_RETRIES

while others look like:

max_retries: int

Can they all have the default please so that things work seamlessly for everyone?

@yz3440
Copy link
Contributor Author

yz3440 commented Jun 24, 2024

@robolyst thanks the for timely feedback. could you please make a new release on pypi after merging this? I have an ongoing project that could use this help to simplify my setup by a bunch.

@robolyst robolyst merged commit 3779698 into robolyst:master Jun 25, 2024
@robolyst
Copy link
Owner

Release going out. Thank you so much for all you improvements 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants