Ticket #2657 (closed enhancement: fixed)

Opened 3 years ago

Last modified 2 years ago

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

Reported by: exarkun Owned by: radix
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.

Change History

Changed 3 years ago by radix

  • owner changed from glyph to radix
  • keywords review added

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

Changed 3 years ago by radix

  • owner radix deleted

Changed 3 years ago by therve

  • owner set to radix
  • keywords review removed
  • cc therve added
  • 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!

Changed 3 years ago by radix

  • owner radix deleted
  • keywords review added

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

Changed 3 years ago by therve

  • keywords review removed
  • owner set to radix

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

Changed 3 years ago 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).

Changed 2 years ago by thijs

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

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

Changed 2 years ago by thijs

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

Note: See TracTickets for help on using tickets.