Opened 5 years ago

Last modified 4 years ago

#5951 defect new

Documentation for the best way to get a reference to a reactor reads as contradictory

Reported by: Ying Li Owned by: Ken
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/clarify_reactor_doscstring-5951-2
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

http://twistedmatrix.com/documents/12.2.0/api/twisted.internet.reactor.html

"Regardless of which reactor is installed, importing this module is the correct way to get a reference to it."

"New application code should prefer to pass and accept the reactor as a parameter where it is needed, rather than relying on being able to import this module to get a reference."

These seem to be contradictory statements. When/where it should be imported rather than passed should be clarified.

Attachments (2)

ticket-5951-docs.patch (2.6 KB) - added by Ken 4 years ago.
ticket-5951-2-docs.patch (2.6 KB) - added by Ken 4 years ago.
v2 patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by teratorn

I guess it means there should be ONE place where you import the reactor (like a twistd plugin file), and then pass it on everywhere else.

comment:2 Changed 5 years ago by Ying Li

(In [35780]) add examples to the docstring for clarity refs #5951

comment:3 Changed 5 years ago by Ying Li

(In [35781]) less passive voice in reactor docstring refs #5951

comment:4 Changed 5 years ago by Ying Li

Branch: clarify_reactor_doscstring_5951
Keywords: easy review added
Owner: Ying Li deleted

comment:5 Changed 5 years ago by Stephen Thorne

Keywords: review removed

Please add a news file before merging.

Approved :)

comment:6 Changed 5 years ago by Stephen Thorne

Owner: set to Ying Li

comment:8 Changed 4 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [35841]) Merge clarify_reactor_doscstring_5951: Elaborate on how to import reactors

Author: cyli Reviewer: jerub Fixes: #5951

Clarify the reactor docstring on how to pass reactors around

comment:9 Changed 4 years ago by Jean-Paul Calderone

Keywords: documentation, easydocumentation easy

Some problems here:

  1. do_something_later and do_something do not follow the naming convention.
  2. def do_something_later(*args, reactor=None, **kwargs): is not valid Python syntax.
  3. import reactor will typically not import the reactor.
  4. do_something_later isn't the way to mark up code in a docstring - C{do_something_later} is.

These are minor, but since there are so many of them it would probably be nice to revert this merge (or else fix them in another ticket today).

comment:10 Changed 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

(In [35872]) Revert r35841 - the example is broken and diverges from the coding standard

Reopens: #5951

comment:11 Changed 4 years ago by Ken

Owner: Ying Li deleted
Status: reopenednew

comment:12 Changed 4 years ago by Ken

Owner: set to Ken
Status: newassigned

Changed 4 years ago by Ken

Attachment: ticket-5951-docs.patch added

comment:13 Changed 4 years ago by Ken

Branch: clarify_reactor_doscstring_5951
Keywords: review added
Owner: Ken deleted
Status: assignednew

This should fix the issues addressed with the reverted change.

comment:14 Changed 4 years ago by Ken

Discussion in person: Default of None needs to go away.

Changed 4 years ago by Ken

Attachment: ticket-5951-2-docs.patch added

v2 patch

comment:15 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/clarify_reactor_doscstring-5951-2

(In [37903]) Branching to clarify_reactor_doscstring-5951-2.

comment:16 Changed 4 years ago by Tom Prince

(In [37906]) Apply ticket-5951-2-docs.patch from wirehead.

Refs: #5951

comment:17 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Ken
  1. The paragraph about default reactors should be split into multiple sentences.
  2. I'm not sure saying anything about multiple reactors makes sense.
  3. "Then the - C{doSomethingLater}": the "-" seems superfluous.
  4. "Also, imported the reactor": importing.
  5. When discussing reactor-already-imported, mentioning twistd *plugins* might be helpful. I was going to suggest that twistd already ensures that (which is true when using --python).
  6. It would be good to reference twisted.internet.task.react.

comment:18 Changed 4 years ago by Itamar Turner-Trauring

Keywords: easy removed
Note: See TracTickets for help on using tickets.