silkapp / rest

Packages for defining APIs, running them, generating client code and documentation.
http://silkapp.github.io/rest
390 stars 52 forks source link

More exhaustive documentation of JSON messages #87

Closed jcpetruzza closed 9 years ago

jcpetruzza commented 9 years ago

Implements the idea in #83, namely it provides examples covering all constructors that can appear, with a cut-off to circumvent infinite schemas.

Also adds a scroll-bar to the modal window, so that larger examples can be displayed.

bergmark commented 9 years ago

This sometimes adds all the generic errors as examples which clutters things up quite a bit:

screen shot 2014-11-14 at 09 15 06

Only the bottom four items are custom reasons for this end point and the rest are the generic errors that can be thrown anywhere.

I think we should filter out anything that isn't a custom reason.

jcpetruzza commented 9 years ago

I could easily add such a filter when generating the examples for the error handler in ActionInfo.hs, but I am not sure if that is the right thing to do.

At least in the docs I see, the long list of non-domain errors only appears in the methods that are derived by rest, e.g., "Update many information". In all the other cases, the output is already limited to domain errors. So the question would be, is there something wrong with the type of errors being returned by derived methods? (e.g., are they of type Reason (Reason e) or something like that?)

bergmark commented 9 years ago

Ah, good catch. I didn't see that. @hesselink what's the type of a multipart error?

hesselink commented 9 years ago

The "update many information" (horrible text, yes :)) is a multi-PUT, which will return a mapping of identifier to either success or failure information. The failure information is indeed a Reason e, so it is embedded in the results here and the output makes sense. I think you'd get the same kind of output if you have a large custom error type, so it might be worth it to think about how to present it better.

jcpetruzza commented 9 years ago

I’m not sure I get it. The multi-PUT returns a mapping of key to success/error, but this goes all to the regular “output”. With the proposed patch, I can indeed see all the possible indexed errors in the sole example for the “output” column.

I would then expect the multi-PUT error output to correspond to a normal method of type, say Reason () (since domain errors are embedded in the regular output anyway) and we should be seeing only one example for the “error” column, namely “[]”.

hesselink commented 9 years ago

Ah, you're right of course. I misunderstood and thought this was the documentation for the output. Let me check what the error type is for the multi-PUT.

hesselink commented 9 years ago

I think it is a bug in the creation of the derived handlers. I've just pushed the derived-handlers-error-dict branch` that might fix this. Can you try and see if it generates more sane documentation?

jcpetruzza commented 9 years ago

Yes, now it generates only one example, with “null” value, that is what I would expect.

Note though that you seem to be adding an XML error ouput even when there is no XML output to begin with. In my case, that makes the doc look funny, but is otherwise not a problem, in case that is hard to fix.

hesselink commented 9 years ago

Thanks for testing, I'll merge that fix into master soon.

Regarding the xml output type: if there are no custom errors at all, we always choose json and xml. If there are custom errors, and they support json and xml, we choose json and xml as well. Would you suggest dropping xml if there are custom errors but they don't support xml?

jcpetruzza commented 9 years ago

Regarding the xml output type: if there are no custom errors at all, we always choose json and xml. If there are custom errors, and they support json and xml, we choose json and xml as well. Would you suggest dropping xml if there are custom errors but they don't support xml?

Yes; I’d probably go as far as producing xml for the errors only if either the input or the output handles xml. But this is all for me a very minor consistency issue in the generated docs; so consider me nitpicking... :)=

hesselink commented 9 years ago

I've just released rest-core-0.33.1.1 which contains the fix for the error dictionaries in multi-handlers.

bergmark commented 9 years ago

Is there a reason this haven't been merged?

hesselink commented 9 years ago

No, I think I just missed it. Will you merge it or shall I?

bergmark commented 9 years ago

i'll merge a bunch of stuff now