i will update the docs when you confirm this is correct

s/recipient/recipients/

s/coma/comma/ (hope none of the recipients get a coma! ☺)

It would be good to document the user_from argument. It would also be good to document the types of these arguments, though I assume they are all basestrings?

For PEP-8, this should be at the top of the file.

I realize you are just moving this code from one place to another place, but I suggest considering breaking the above function into a few helpers so it's not quite so long. It'll make it easier to understand, and also easier to test.

This looks good to me, though I suggest testing the change if feasible.

That's because, i have to import this file (lib.tasks) in lib.notify :/

Circular imports are a sign that there is an organizational problem in the code. If module A needs module B and module B needs module A, it is likely that module A and B should either be the same module, or that some item or items from them should be brought into a third module C that they both use. I suggest refactoring to fix this.

okay. I tried moving all the helper functions to different file - pagure.lib.task_helpers still i am facing circular imports :/ . There are two methods of lib.notify which are being used in task_helpers. Those two methods are also being used in lib.notify(more often). :/

methods are: _build_url and _fullname_to_url (i thought of making it public but that's different.)

To clarify, I'm not demanding and not necessarily even recommending fixing the circular import problem in this PR, unless this PR is introducing the circular nature (and even then I'm not demanding it ☺). If the circular nature pre-exists this PR, I say it's best left as a separate concern (and a separate PR). However, if this PR is introducing circular dependencies I highly recommend addressing that before merging as it indicates an organizational problem.

To clarify, I'm not demanding and not necessarily even recommending fixing the circular import problem in this PR, unless this PR is introducing the circular nature (and even then I'm not demanding it ☺). If the circular nature pre-exists this PR, I say it's best left as a separate concern (and a separate PR). However, if this PR is introducing circular dependencies I highly recommend addressing that before merging as it indicates an organizational problem.

Chill, i take your words seriously.

Should we rebase?

I lost :(

Modules don't need a shebang :)

Editor just puts it there, i will remove

@pingou anything else?

Missing a header no?

This shouldn't be there, if you re-configure the logger while it was already configured before, you're going to loose all the info (been there done that)

author?

Not sure I would have moved these two functions (and then this file would be a mailer_helper maybe?)

Couple of comments but this is looking nice.

My only concern is about the workers themselves, I've noticed it sometime takes a while to get a PR merged for example and I think that's because the workers are just a little overloaded, so I would prefer to wait a little before merging this PR so we put in place some monitoring of the workers' load and see if we can spin up some more. Otherwise receiving emails might end up being delayed.

@vivekanand1101 is this something you are still interesting to bring in?

Sorry for dropping the ball on you :(

@pingou Oh man, this is probably a really good idea to do, how difficult would it be to rebase this and bring it in now?

@ngompa is it still relevant and something you want to bring in after 4 years? If so I can grab what's already in the PR and create a new one from it.

I think generally we want all our async stuff going through celery, especially so the application doesn't bottleneck itself on sending emails. So yeah, we probably still want this.

This project has moved to https://forge.fedoraproject.org/apps/pagure. As part of the migration, all open pull requests on pagure.io have been closed. If you'd like to continue working on this, please fork the repository on the new forge and re-submit your PR there.