Opened 10 years ago
Last modified 4 years ago
#5532 enhancement new
AMP Command specification format
Reported by: | teratorn | Owned by: | teratorn |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | amp |
Cc: | Branch: |
branches/amp-json-spec-5532-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | teratorn |
Description (last modified by )
Proposing a standard convention for specifying AMP Command structures for sharing between multiple applications, with clients and servers using different implementations of AMP in different languages.
JSON is chosen as the basic serialization format for maximum portability and ease of parsing.
An example of the proposed format:
{"commands" : { "Sum" : { "arguments" : [["a", "Integer"], ["b", "Integer"]], "response" : [["total", "Integer"]] }, "Divide" : { "arguments" : [["numerator", "Float"], ["denominator", "Float"]], "response" : [["result", "Float"]], "errors" : [["ZERO_DIVISION", "ZeroDivisionError"]] }, "SumList" : { "arguments" : [["args", {"type" : "ListOf", "of" : "Integer"}], ["basevalue", {"type" : "Integer", "optional" : true}] ], "response" : [["total", "Integer"]] }, "SumPairs" : { "arguments" : [["args", {"type" : "TupleList", "of" : ["Integer", "Integer"]}]], "response" : [["total", "Integer"]] } } }
Which, when parsed and loaded results in access to Command classes equivalent to:
class Sum(amp.Command): arguments = [['a', amp.Integer()], ['b', amp.Integer()]] response = [['total', amp.Integer()]] class Divide(amp.Command): arguments = [['numerator', amp.Float()], ['denominator', amp.Float()]] response = [['result', amp.Float()]] errors = [['ZERO_DIVISION', ZeroDivisionError]] class SumList(amp.Command): arguments = [['args', amp.ListOf(amp.Integer())], ['basevalue', amp.Integer(optional=True)]] response = [['total', amp.Integer()]] class SumPairs(amp.Command): arguments = [['args', TupleList(amp.Integer(), amp.Integer())]] response = [['total', amp.Integer()]]
Change History (15)
comment:1 Changed 10 years ago by
Author: | → teratorn |
---|---|
Branch: | → branches/amp-json-spec-5532 |
comment:2 Changed 10 years ago by
Keywords: | review added |
---|
Ready for review and feedback on JSON format
comment:3 Changed 10 years ago by
Owner: | teratorn deleted |
---|
comment:5 Changed 10 years ago by
Keywords: | review removed |
---|
A few thoughts:
fromFile
seems like it oughta be a class method that creates a Spec.- Seems kinda weird to have the type mapping defined in the code loading the spec. Should there be a default one that you can override as needed?
Review comments:
SpecTestCase
andSpecIntegrationTestCase
need docstrings.- Test methods in
SpecTestCase
need docstrings. MyAMP.sum
inSpecIntegrationSetupTestCase.setUp
needs whitespace around+
.- Coding standard says to put multiline
addCallbacks
etc chaining in parens and put dots at the start of the line. - The integration tests make a connection over loopback. How hard would it be to use a fake transport?
- You're calling
str()
on a unicode object inSpec._loadCommand
. This seems likely to surprise someone or at the very least tempt someone to change it later. If you really mean "convert to ASCII always without question" thentypeSpec.encode("ascii")
seems like a better choice. Alternately you could just leave it as a unicode string. and not callstr
.
- Oh, you're doing it in
_parseArgSpec
and_resolveType
too.
comment:6 Changed 10 years ago by
Owner: | set to teratorn |
---|
comment:7 Changed 10 years ago by
A few extra notes:
- Can we replace the name Spec too?
- And maybe fromFile could be fromJSONFile? Or maybe fromJSON, and it could take a string instead of a file. Or even an already parsed json structure?
FilePath
is better thanos.path
for the unit tests.- trial provides
assertIsInstance
- coding standard calls for
assertEqual
and notassertEquals
. - There's a handful of other places where the tests don't follow the coding standard, too.
comment:8 Changed 9 years ago by
Branch: | branches/amp-json-spec-5532 → branches/amp-json-spec-5532-2 |
---|
(In [37600]) Branching to 'amp-json-spec-5532-2'
comment:9 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Keywords: | review added |
Owner: | teratorn deleted |
I've addressed all the review comments in the current branch. My eyes can't pick out any more coding standards violations, but that doesn't mean I caught them all..
I've changed the proposed JSON format such that Command names are top-level keys under the "commands" key. This seems more natural to me (thanks dreid), and should map more cleanly to formats like YAML where indentation indicates structure.
Ready for review.
comment:10 Changed 9 years ago by
comment:11 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to teratorn |
- Comments on the specification format.
- The exception names look python specific to me, which means that the specification isn't entirely language neutral. I'm not sure how best to address this.
- A related point, the format is unversioned. It would probably be good to give some thought on how it can be versioned.
- (followup tickets) The format can easily be turned into amp boxes
- It would be reasonable to define amp types for holding command specifications.
- Using the above specification, it should be possible to make amp responders introsepctable. That would make things like dash's json-to-amp bridge workable.
- There is a comment about the command name needing to be a valid python identifier.
- The amp implementation doesn't seem to enforce this, it accepts arbitrary bytestrings.
- The code in question actually just enforces that name be valid ascii.
- It is probably reasonable to limit it somehow. But we should be clear in what we require, and test that we require exactly that.
- The format of specifications should be documented (in
doc/core/specifications/
perhaps)? 'of'
seems somewhat ad-hoc. The current paramaterized types are containers, soof
makes sense. Is that always going to be the case? I think the design could be improved here.
- Comments on the implementation.
- Should the mappings for types and errors be seperate?
- There should also be some usage information in the amp howto.
- An instance of
SchemaLoader
doesn't actually load anything. It seems perhaps like it might be more appropriate to call itSchema
?.fromJSON
is actually the loader.- This seems like it might relate to 1.4.1
- If this is called
Schema
, one might want to create instances from existing commands, rather than loading from a file. Relatedly,typeMapping doesn't seem like a relevant
initargument for a
Schmea`.
- I'm wary of trying to protect the user from modifying defaultTypeMapping, at least in the ad-hoc way it is done here. A quick look at
help(dict)
suggested a number of ways of evading the protection done in the branch.- I'm not opposed to having a frozen dict type, but I'm wary of putting a partial hack like this in.
- There doesn't appear to be any way to inspect the loaded commands, other than looking at
dir(spec)
. - (followup ticket) There doesn't seem to be any way to create a JSON specification from existing Command classes.
SchemaLoaderIntegrationTestCase.test_sum_call
should usesuccessResultOf
- The name doesn't follow the coding standard either.
SchemaLoaderTestCase
: The test docstrings should specify what 'correctly' means.- But, it might be easier to test smaller units of functionality.
- We don't care
json.loads
works. - The command and argument parsing can probably be tested individally.
- We don't care
- But, it might be easier to test smaller units of functionality.
comment:12 follow-up: 14 Changed 9 years ago by
I wonder if there's any hope of getting this released as a third-party library before it's integrated into Twisted. I would love to try it out and provide feedback, and it's a lot more likely I'll be able to try out a separately released project rather than a development branch of Twisted.
comment:13 follow-up: 15 Changed 9 years ago by
I'll second that suggestion. It does seem like something that could benefit from a short release cycle. (This seems like a common theme, with code like this. Perhaps something worth making more explicit?)
comment:14 Changed 4 years ago by
Replying to Jean-Paul Calderone:
I wonder if there's any hope of getting this released as a third-party library before it's integrated into Twisted. I would love to try it out and provide feedback, and it's a lot more likely I'll be able to try out a separately released project rather than a development branch of Twisted.
if it's my ticket or branch, feel free not to wait, and branch it on github or wherever.
I would like to work more on this, but AMP isn't exactly a priority at the moment..
comment:15 Changed 4 years ago by
Replying to Tom Prince:
I'll second that suggestion. It does seem like something that could benefit from a short release cycle. (This seems like a common theme, with code like this. Perhaps something worth making more explicit?)
note my reply above - a lot of "small" things in twisted really would benefit from being quickly worked on outside of Twisted trunk - what do you suppose is an appropriate workflow these days? it's probably easier to just collaborate on github now than uh 7 years ago?
(In [33707]) Branching to 'amp-json-spec-5532'