Opened 6 years ago

Closed 6 years ago

#6389 enhancement closed fixed (fixed)

twisted.internet.task.react should allow unprovided argv

Reported by: Julian Berman Owned by: Jonathan Jacobs
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/react-no-argv-6389
branch-diff, diff-cov, branch-cov, buildbot
Author: jonathanj

Description


Attachments (2)

react-argv.patch (2.9 KB) - added by Julian Berman 6 years ago.
react-argv.2.patch (3.4 KB) - added by Julian Berman 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Julian Berman

Attachment: react-argv.patch added

comment:1 Changed 6 years ago by Julian Berman

It seems unnecessary to need to do react(main, []). Many APIs (including some in Twisted) that are delegating to other callables will use a default arg of () in place of argv if there aren't any args to pass along in a degenerate case, so it seems reasonable to expect to not need to pass it here.

comment:2 Changed 6 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Julian Berman
  1. Please update the docstring for argv, possibly to mention that if a value is not provided it defaults to an empty tuple.
  1. Please add a news file.

Something that bothers me: There are no new tests for this and the old tests that used to pass [] have been changed. Maybe this is okay? But it's probably bothering me for a reason, I just don't know what it is.

Changed 6 years ago by Julian Berman

Attachment: react-argv.2.patch added

comment:3 Changed 6 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

Hey, thanks for the review.

Fixed the docstring. Had a topfile but I always forget git add -N so it didn't come through :/. Sorry.

It should bother you that now these tests don't cover the case where there's an arg provided, but I verified when making the change that there's another test that specifically test an arg being passed. Other than that, I think it's appropriate here.

comment:4 Changed 6 years ago by Jonathan Jacobs

Author: jonathanj
Branch: branches/react-no-argv-6389

(In [37965]) Branching to 'react-no-argv-6389'

comment:5 Changed 6 years ago by Jonathan Jacobs

(In [37966]) Apply react-argv2.patch, refs #6389.

comment:6 Changed 6 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Julian Berman

This looks good to merge.

Build results

comment:7 Changed 6 years ago by Julian Berman

Owner: changed from Julian Berman to Jonathan Jacobs

comment:8 Changed 6 years ago by Jonathan Jacobs

Resolution: fixed
Status: newclosed

(In [37991]) Merge react-no-argv-6389

Author: Julian Reviewer: jonathanj Fixes: #6389

Allow omitting the "argv" argument, to mean no additional arguments, when calling twisted.internet.task.react.

Note: See TracTickets for help on using tickets.