Opened 4 years ago

Closed 3 years ago

#6071 defect closed fixed (fixed)

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

Reported by: habnabit Owned by: Richard Wall
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/react-reactor-arg-6071
branch-diff, diff-cov, branch-cov, buildbot
Author: reecer

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 4 years 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 Adam Dangoor 3 years ago.
Addresses tomprince's comments
react-reactor-arg-6071-v2.patch (1.3 KB) - added by Adam Dangoor 3 years ago.
Like react-reactor-arg-6071-v1.patch, but has a top file

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by habnabit

Keywords: documentation added
Type: enhancementdefect

comment:2 Changed 4 years ago by Richard Wall

Keywords: easy added

comment:3 Changed 4 years ago by reecer

Author: 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 4 years ago by reecer

Attachment: task_docstring.patch added

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

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

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 4 years ago by Tom Prince

Author: reecerreecer, tomprince
Branch: branches/react-reactor-arg-6071

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

comment:6 Changed 4 years ago by Tom Prince

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

Refs: #6071.

comment:7 Changed 4 years ago by Tom Prince

Author: reecer, tomprincereecer
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 3 years ago by Adam Dangoor

Addresses tomprince's comments

comment:8 Changed 3 years ago by Adam Dangoor

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 3 years ago by Adam Dangoor

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 3 years ago by Adam Dangoor

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

comment:10 Changed 3 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:11 Changed 3 years ago by Richard Wall

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

comment:12 Changed 3 years ago by Richard Wall

Keywords: review removed
Status: assignednew

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 3 years ago by Richard Wall

Resolution: fixed
Status: newclosed

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