Opened 8 years ago

Closed 8 years ago

#2273 defect closed fixed (fixed)

AMP documentation on command errors is reversed or code is

Reported by: htj Owned by:
Priority: normal Milestone: Core-2.5
Component: core Keywords: amp
Cc: Branch:
Author: Launchpad Bug:

Description

line 1038 in protocols/amp.py says:

@cvar errors: A mapping of error tag to exception type.

which is wrong because errors have to be mapped like this to work:

errors = {SomeException: 'SOME_ERROR'}

The easy solution is to flip the text in the docstring so it reads:

@cvar errors: A mapping of exceptions types to error tags.

However, I think that what the docstring says is better. By having ERROR_TAG -> Exception, it is possible to map several error tags to the same exception. This is not possible with the current scheme, as an exception maps to an error tag. Hence one exception cannot map to several tags, unless we start using lists, but that just makes it more nasty.

I know this is an API change, but since AMP has not yet been in official release it should be doable.

Change History (6)

comment:1 Changed 8 years ago by exarkun

  • Milestone set to Twisted-2.5

comment:2 Changed 8 years ago by glyph

  • Owner changed from glyph to radix

You should do something about this, either changing the docs or the code.

comment:3 Changed 8 years ago by radix

  • Keywords review added
  • Owner changed from radix to mithrandi

Ready for review in fix-amp-docstring-2273

For the reviewer's convenience, I include the entire changeset here:

-    @cvar errors: A mapping of error tag to exception type.
+    @cvar errors: A mapping of exception type to error tag.

comment:4 Changed 8 years ago by mithrandi

  • Keywords review removed
  • Owner changed from mithrandi to radix

As best I can tell, this change is correct. Go ahead and merge.

comment:5 Changed 8 years ago by radix

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

(In [18850]) Merge fix-amp-docstring-2273

Author: radix
Reviewer: mithrandi
Fixes #2273

One is meant to pass a mapping of exception types to tags
as the 'errors' argument to Command, not a mapping of tags to exception
types.

comment:6 Changed 4 years ago by <automation>

  • Owner radix deleted
Note: See TracTickets for help on using tickets.