Opened 10 years ago

Closed 10 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:


line 1038 in protocols/ 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 10 years ago by exarkun

  • Milestone set to Twisted-2.5

comment:2 Changed 10 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 10 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 10 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 10 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 6 years ago by <automation>

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