Skip to content

Conversation

@lemontree210
Copy link
Member

@lemontree210 lemontree210 commented Jul 12, 2023

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.

@lemontree210 lemontree210 self-assigned this Jul 12, 2023
@lemontree210 lemontree210 added the ⚙️ documentation affected functionality: documentation label Jul 12, 2023
Comment on lines 1099 to 1100
:any:`Custom Webhook Bot Example <examples.customwebhookbot>`),
make sure that you explicitly call this method.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: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?

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

@lemontree210 lemontree210 Jul 14, 2023

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

@lemontree210 lemontree210 changed the title add notes on running post_ methods explicitly add notes on running functions instead of setting post_ methods Jul 14, 2023
Copy link
Member

@harshil21 harshil21 left a 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.

@lemontree210
Copy link
Member Author

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).

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 await my function manually. I guess it's once again about our expectations from the user and how much "superfluous" (with or without quotation marks) information we want to give them in our docs. I definitely see that what is said in the note can be inferred from the info already present.

@lemontree210
Copy link
Member Author

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.

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)?

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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 …

@harshil21 harshil21 added the 📋 pending-merge work status: pending-merge label Aug 1, 2023
@Bibo-Joshi Bibo-Joshi merged commit 9fe836a into doc-fixes Aug 2, 2023
@Bibo-Joshi Bibo-Joshi deleted the add-note-on-post-init branch August 2, 2023 08:31
@Bibo-Joshi Bibo-Joshi mentioned this pull request Aug 2, 2023
3 tasks
@harshil21 harshil21 removed the 📋 pending-merge work status: pending-merge label Aug 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⚙️ documentation affected functionality: documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants