Ticket #2657 (closed enhancement: fixed )

Opened 2 years ago

Last modified 1 year ago

twisted.protocols.amp.Command should have an response parsing method parallel to makeResponse

Reported by: exarkun Assigned to: radix
Type: enhancement Priority: highest
Milestone: Component: core
Keywords: Cc: therve
Branch: branches/err-docstring-2657 Author: thijs
Launchpad Bug:

Description

makeResponse is useful for unit testing response sending. A similar method for parsing would be useful for testing response receiving.

Attachments

Change History

  2007-05-20 21:52:41+00:00 changed by radix

  • keywords set to review
  • owner changed from glyph to radix

Ok, this is up for review in amp-factoring-2657+2667

Note to reviewers: In addition to fixing this ticket, it also fixes #2667, #2656, and #2664. Please make sure you're happy with this branch in relation to those tickets. (don't worry, #2656 and #2664 are very minor documentation improvements).

  2007-05-20 22:01:10+00:00 changed by radix

  • owner deleted

  2007-05-22 10:28:58+00:00 changed by therve

  • cc set to therve
  • keywords deleted
  • owner set to radix
  • priority changed from normal to highest
  • The imports of succeed and PeerVerifyError can removed from test_amp, as top imports are already made.
  • MyBox should have the good format for docstrings, and the pass statement could be removed.
  • The ivar of NoNetworkProtocol (parse_arguments, parse_response,make_arguments) should renamed to mixedCase (I guess they should be renamed to not collide with other method name?).
  • The docstrings of the methods of ProtocolIncludingArgument aren't more useful than the code itself
  • The copyrights of both files are bit strange. I don't really mind that Divmod appears, but perhaps Twisted should appear too.

Overall, that looks really good, nothing more to add.

Thanks!

  2007-05-22 22:39:09+00:00 changed by radix

  • keywords set to review
  • owner deleted

All reviewer comments implemented. Thank for the review. Back to review!

  2007-05-23 07:45:41+00:00 changed by therve

  • keywords deleted
  • owner set to radix

Great, just remove the 2001 copyright from test_amp as exarkun doesn't like retro-copyright, then merge.

  2007-05-23 15:37:25+00:00 changed by radix

  • status changed from new to closed
  • resolution set to fixed

(In [20391]) Merge amp-factoring-2657+2667.

Author: radix Reviewer: therve Fixes #2657 Fixes #2667 Fixes #2656 Fixes #2664

It is now possible to use public methods on Command objects to serialize and parse arguments and responses based on that Commandn's arguments and response schemas. It was already possible to serialize responses via the Command in this way; analogs for the other three cases were added. (#2657 & #2667).

Some docstrings in nearby code were alse fixed in this branch. (#2656 & #2664).

  2008-08-05 22:30:08+00:00 changed by thijs

  • branch set to branches/err-docstring-2657
  • author set to thijs

(In [24535]) Branching to 'err-docstring-2657'

  2008-08-05 22:43:37+00:00 changed by thijs

(In [24535]) Branching to 'err-docstring-2657'

Note: See TracTickets for help on using tickets.