Opened 2 years ago

Last modified 17 months ago

#5951 defect new

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

Reported by: cyli Owned by: wirehead
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/clarify_reactor_doscstring-5951-2
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

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 wirehead 18 months ago.
ticket-5951-2-docs.patch (2.6 KB) - added by wirehead 18 months ago.
v2 patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 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 2 years ago by cyli

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

comment:3 Changed 2 years ago by cyli

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

comment:4 Changed 2 years ago by cyli

  • Branch set to clarify_reactor_doscstring_5951
  • Keywords easy review added
  • Owner cyli deleted

comment:5 Changed 2 years ago by jerub

  • Keywords review removed

Please add a news file before merging.

Approved :)

comment:6 Changed 2 years ago by jerub

  • Owner set to cyli

comment:8 Changed 2 years ago by cyli

  • Resolution set to fixed
  • Status changed from new to closed

(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 2 years ago by exarkun

  • Keywords changed from documentation, easy to documentation 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 2 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Reopens: #5951

comment:11 Changed 18 months ago by wirehead

  • Owner cyli deleted
  • Status changed from reopened to new

comment:12 Changed 18 months ago by wirehead

  • Owner set to wirehead
  • Status changed from new to assigned

Changed 18 months ago by wirehead

comment:13 Changed 18 months ago by wirehead

  • Branch clarify_reactor_doscstring_5951 deleted
  • Keywords review added
  • Owner wirehead deleted
  • Status changed from assigned to new

This should fix the issues addressed with the reverted change.

comment:14 Changed 18 months ago by wirehead

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

Changed 18 months ago by wirehead

v2 patch

comment:15 Changed 18 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/clarify_reactor_doscstring-5951-2

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

comment:16 Changed 18 months ago by tomprince

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

Refs: #5951

comment:17 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to wirehead
  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 17 months ago by itamar

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