-
Notifications
You must be signed in to change notification settings - Fork 6k
add notes on running functions instead of setting post_ methods
#3797
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
Conversation
telegram/ext/_applicationbuilder.py
Outdated
| :any:`Custom Webhook Bot Example <examples.customwebhookbot>`), | ||
| make sure that you explicitly call this method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| :any:`Custom Webhook Bot Example <examples.customwebhookbot>`), | |
| make sure that you explicitly call this method. | |
| :any:`Custom Webhook Bot Example <examples.customwebhookbot>`),. |
There is not really a need to set the function on the ApplicationBuilder in this case. you can just manually do await some_function(application) if you're doing custom logic anyways :)
Can we use a sphinx insertion thingy to avoid repitition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... makes sense :)
As for the insertion - it doesn't work if I mention methods there. Sphinx complains about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, you'll have to use the full name then, see e.g. :meth:~telegram.ext.Application.run_polling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done. It didn't like the :any: link to the example either, so I had to put an https hyperlink instead
post_ methods explicitlypost_ methods
harshil21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I think the note is redundant. The opening paragraph in post_* methods already tell that it is only executed by run_polling/webhook, and clicking on that link shows the order of execution ("The order of run_polling is as follows" line).
How do you feel about raising a warning if app.start() was called and app.post_init() was set but not called? This would need additional logic to work (some class variables we should set when the function has finished running?), so it might be worth discussing if we even want that.
OK, I see what you mean. I actually don't insist on adding my note. I realize now that by following this path, I could have figured out earlier that I have to |
That's an interesting idea, similar to a warning when user has declared some states but is not using some of them. I too think, however, that the implementation will be rather tedious. Do we have any idea about how many users actually implement this custom logic (e.g. because of several webhooks like in my case)? |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the proposed content changes as they are and I personally don't find them way too verbose (although @harshil21 ofc made a valid point).
I would guess that programmatically checking and issuing a warning is not worth the effort …
As discussed in dev group, I added a note stating that in cases like custom webhook example user should explicitly call their own functions instead of setting
post_methods.