Opened 2 years ago

Closed 11 months ago

#6071 defect closed fixed (fixed)

t.i.task.react's docstring doesn't mention the reactor argument

Reported by: habnabit Owned by: rwall
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/react-reactor-arg-6071
(diff, github, buildbot, log)
Author: reecer Launchpad Bug:

Description

The docstring for t.i.task.react says:

    @param main: A callable which returns a L{Deferred}.  It should take as
        many arguments as there are elements in the list C{argv}.

But main is called as:
finished = main(_reactor, *argv)

The docstring should be updated to reflect that main will be called with the reactor as an argument.

Attachments (3)

task_docstring.patch (742 bytes) - added by reecer 18 months ago.
Updated the docstring for the PARAM "main" to include that the PARAM _reactor is the first argument when calling "main."
react-reactor-arg-6071-v1.patch (1.1 KB) - added by adamtheturtle 11 months ago.
Addresses tomprince's comments
react-reactor-arg-6071-v2.patch (1.3 KB) - added by adamtheturtle 11 months ago.
Like react-reactor-arg-6071-v1.patch, but has a top file

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by habnabit

  • Keywords documentation added
  • Type changed from enhancement to defect

comment:2 Changed 19 months ago by rwall

  • Keywords easy added

comment:3 Changed 18 months ago by reecer

  • Author set to reecer

Changed the docstring to the following:

    @param main: A callable which returns a L{Deferred}.  It should take as
        many arguments as there are elements in the list C{argv}. Called with 
        _reactor as first parameter.

Changed 18 months ago by reecer

Updated the docstring for the PARAM "main" to include that the PARAM _reactor is the first argument when calling "main."

comment:4 Changed 18 months ago by itamar

  • Keywords review added

Thanks for the patch! Make sure you add the "review" keyword when submitting a patch to Twisted, so we know to look at it.

comment:5 Changed 18 months ago by tomprince

  • Author changed from reecer to reecer, tomprince
  • Branch set to branches/react-reactor-arg-6071

(In [38461]) Branching to react-reactor-arg-6071.

comment:6 Changed 18 months ago by tomprince

(In [38462]) Apply task_docstring.patch from reecer.

Refs: #6071.

comment:7 Changed 18 months ago by tom.prince

  • Author changed from reecer, tomprince to reecer
  • Keywords review removed
  • Owner set to reecer

Thanks for your contribution, and sorry for the delay in responding to it.

Your change is an improvement on the current decription, but it still isn't entirely clear how main gets called. It should perhaps say something about taking reactor, followed by the elemnts of argv.

One thing that the current wording doesn't make clear, is that main can have optional and *args as well.

(Please submit further patches against the code in the branch)

Changed 11 months ago by adamtheturtle

Addresses tomprince's comments

comment:8 Changed 11 months ago by adamtheturtle

  • Keywords review added
  • Owner reecer deleted

Please review react-reactor-arg-6071-v1.patch which adds a clearer explanation of "react"'s main argument, and an example main function.

comment:9 Changed 11 months ago by adamtheturtle

Please review react-reactor-arg-6071-v2.patch, which (on top of the improvements in review react-reactor-arg-6071-v1.patch) has a news file.

Changed 11 months ago by adamtheturtle

Like react-reactor-arg-6071-v1.patch, but has a top file

comment:10 Changed 11 months ago by rwall

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

comment:11 Changed 11 months ago by rwall

(In [40932]) Apply react-reactor-arg-6071-v2.patch from adamthturtle. Refs #6071

comment:12 Changed 11 months ago by rwall

  • Keywords review removed
  • Status changed from assigned to new

Thanks Adam,

Looks good. The only thing I notice is that the new example doesn't really belong in the bulleted list.

I'll reformat that and merge.

comment:13 Changed 11 months ago by rwall

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

(In [40934]) Merge react-reactor-arg-6071

Authors: reecer, adamtheturtle
Reviewers: tom.prince, rwall
Fixes: #6071

The docstring for twisted.internet.task.react now better documents the main
parameter.

Note: See TracTickets for help on using tickets.