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

adding sleep to load image in case of bulk messages #93

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arsalankhan994
Copy link

No description provided.

Copy link
Collaborator

@euriconicacio euriconicacio left a comment

Choose a reason for hiding this comment

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

Hi, @arsalankhan994. First of all, thanks a lot for contributing!

One question: any reason for an exact 0.3 sleep? It seems like a really specific value. Can you test + provide further info if that fits all possible cases you are attempting to address?

Thanks a lot!

@arsalankhan994
Copy link
Author

arsalankhan994 commented Dec 9, 2022

Hi @euriconicacio, I hope you are well. The problem that I was facing is that when I called send_picture inside the loop, then some of the images are not sent properly and shows an error icon because the browser close immediately after sending an image. So I tested this same case with 0.1 sleep, then 0.2 and then 0.3, and it perfectly worked on 0.3 seconds. I have tested this same thing with different images and attachments and I think that it should work in all cases. Thanks

@euriconicacio
Copy link
Collaborator

Hi @euriconicacio, I hope you are well. The problem that I was facing is that when I called send_picture inside the loop, then some of the images are not sent properly and shows an error icon because the browser close immediately after sending an image. So I tested this same case with 0.1 sleep, then 0.2 and then 0.3, and it perfectly worked on 0.3 seconds. I have tested this same thing with different images and attachments and I think that it should work in all cases. Thanks

Hi, @arsalankhan994! Thanks for the reply.
Got your intent over there, thanks a lot for your contribution. One small consideration + one small request:

  1. One of our current efforts focuses on implementing unit tests and rolling this into TDD. So, we are being a little more strict with contributions triage - thanks a lot for the comprehension on this topic, btw;

  2. As a consequence, considering the current non-unit-tests state, could you please perform three tests and provide the output for them (for output, consider time to run and sleep length employed)?

    • The first test, with maximum attachment size (slightly lower than 16MB - maybe 15.9MB) on your current computer+internet connection configuration;
    • The second one, with your current file size and optimized (as much as possible, ideally measured) internet connection configuration;
    • The third one with mixed first and second tests - maximum attachment size and optimized internet connection.

Please, let us know if that's not possible, so we can pull your branch and run them. Thanks!

Copy link
Collaborator

@euriconicacio euriconicacio left a comment

Choose a reason for hiding this comment

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

Additional tests were requested in a separate comment. Main goal: use the ideal value for the proposed sleep.

Btw, @arsalankhan994, can you please set that as a global variable? Thanks.

@euriconicacio
Copy link
Collaborator

Hi, @arsalankhan994. Any update on the requested test outputs? Thanks.

Hi, @b98430. I will be deleting your comment from this PR. Please, address it as an issue. Thanks.

Repository owner deleted a comment from b98430 Jan 20, 2023
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