Skip to content

Conversation

@mullerivan
Copy link

🎱

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 89.873% when pulling bbddb20 on mullerivan:master into fd93133 on dokterbob:master.

change it to what ever you feel is good for your company::

DELAY_BETWEEN_EACH_EMAIL = 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure here the number is seconds and can be factional (for 10000 emails waiting 0.1 or 1 second makes a huge difference. Also, if you can, please write Email as email.

@dokterbob
Copy link
Collaborator

Ok, reviewed it. It looks fine now, except that it would be nice to have a little test for the added functionality, so that it actually waits a specified amount of time (like 0.001 s). (Later, ideally, we would mock patch time.sleep() so to remove the delay.)

To update the pull request you don't need to create a new one. Just create a branch for this pull request (name it whatever makes sense to you) and make the requested modifications there. Change the pull request to match the branch. Every time the branch is updated, the pull request is updated too.

To create a branch: git checkout -b funnybranchname
To push it git push -u origin funnybranchname
(-u makes sure that afterwards you can just do git push without specifiying the branch)

If you want to get really fancy you can start rebasing, see https://stackoverflow.com/questions/9790448/how-to-update-a-pull-request for an example of how to do that.

@mullerivan
Copy link
Author

I'm not 100% sure how to test the delay between emails......

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 89.873% when pulling 5206c59 on mullerivan:master into fd93133 on dokterbob:master.

@dokterbob
Copy link
Collaborator

Code looks great now, thanks @mullerivan!

To test this feature, you could write a variant of the submission test where you measure the time it takes to submit to two recipients first without an EMAIL_DELAY and then with EMAIL_DELAY set to a small but measurable value (i.e. 0.1 s).

Then you assert that the difference in execution time is roughly equal to the configured delay.

@dokterbob
Copy link
Collaborator

This has been implemented and merged in #223.

@dokterbob dokterbob closed this Nov 11, 2017
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.

3 participants