launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

[0]

> The API code seems labyrinthine, but I know there's history there, and
> an awful lot of this was done for 12.04, under extreme time pressure.
> In other words, please don't take any of the following personally.

I agree 100%.  The main problem is that, like you say, some of the definition of the API is wound up in its implementation and that is really bad.  The fact that this code is a bit cryptic is also a problem but it's fairly well tested and, in a sense, well isolated from the work that we usually have to preform on the API (i.e. adding API methods, changing API methods).

> However, @api_operations needs an *HTTP* method when decorating an
>  *instance* method, and the handler's allowed_methods property takes
>  a list of HTTP methods too.

Not sure I understand your point here, care to elaborate a bit?

>- Things like get_mandatory_param and get_optional_list are used
>  instead of declaring the API's interface.

>- Going further: the only definition of the API is wound up in its
>  implementation.

That is indeed ugly :/

[1]

120	-    global _API_DOC
121	-    if _API_DOC is None:
122	-        _API_DOC = generate_api_doc()

Why did you remove that caching bit?  Maybe (but this is unrelated to the change you're doing here) we should use Django's cache instead of using module-level variable though.

[2]

143	+        "doc": "MAAS API",
144	+        "handlers": [
145	+            describe_handler(handler)
146	+            for handler in find_api_handlers(module)
147	+            ],

The API version seems like a very important information to have here.
-- 
https://code.launchpad.net/~allenap/maas/inspect-api/+merge/123853
Your team MAAS Maintainers is requested to review the proposed merge of lp:~allenap/maas/inspect-api into lp:maas.