Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Swagger Documention To The API #2227

Merged
merged 3 commits into from
Sep 7, 2017
Merged

Add Swagger Documention To The API #2227

merged 3 commits into from
Sep 7, 2017

Conversation

arottersman
Copy link

@arottersman arottersman commented Sep 6, 2017

Overview

  • Adds django-rest-swagger to the project
  • Documents inputs, outputs and descriptions of each of the AOI endpoints (the /analyzes and /rwd)
  • Let's not worry too much about the exact content. The intent is to
    • make it easy for us to demo, and for the client to check on
    • provide an initial place to stick the docs

Connects #2185

Demo

7h3fjjgnmq

Notes

  • The example outputs make the docstrings real lengthy. I'll open an issue to look into moving them out of the implementation files, but for now we can make like @rajadain and set our editors to auto-collapse them.

  • The example responses are shown as regular json, but the response currently returns stringified json. We'll fix the response type in Geoprocessing API: Responses should be proper json #2200

Testing Instructions

  • vagrant ssh app -c 'cd /vagrant/src/mmw/requirements && sudo pip install -r development.txt'
  • ./scripts/manage.sh collectstatic
  • go to localhost:8000/api/docs/
  • Test out each endpoint.

Here's an example analyze aoi:

{
  "type": "MultiPolygon",
  "coordinates": [[
    [
     [
      -74.97739791870116,
      40.08164651013995
    ],
    [
      -74.9734228849411,
      40.08164651013995
    ],
    [
      -74.9734228849411,
      40.08403526674243
    ],
    [
      -74.97739791870116,
      40.08403526674243
    ],
    [
      -74.97739791870116,
      40.08164651013995
    ]
    ]
 ]]
}

Alice Rottersman added 2 commits September 6, 2017 16:24
* add django-rest-swagger to document the public API
* add markdown to allow using markdown in the docstrings that show up in swagger
* setup an initial swagger config with docs available at /api/docs
@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

Taking a look now.

@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

In addition to sudo pip install -r requirements.txt, I also had to run ./scripts/manage.sh collectstatic to get the docs to show up correctly.

@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

Hmm, the markdown isn't rendering correctly for me:

image

@arottersman
Copy link
Author

Thanks, I've updated the testing instructions to include the collectstatic.

Not sure what's up with the markdown. Maybe check if the markdown package installed?

@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

I do seem to have it:

vagrant@app:/vagrant/src/mmw/requirements$ sudo pip list | grep -i markdown
Markdown (2.6.9)

@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

A hard refresh fixed it. It may have been a leftover from when I loaded the page with bad JavaScript before running collectstatic.

@rajadain
Copy link
Member

rajadain commented Sep 7, 2017

This works well. Not sure where the api-docs URL is coming from:

image

Copy link
Member

@rajadain rajadain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 tested. This works well, and reads well enough. A number of editors allow collapsing docstrings, like IntelliJ IDEA:

image

so I think long docstrings are fine. We should try to keep indentation consistent throughout the file though, to ease tab navigation.

"displayName": "Land",
"name": "land",
"categories": [
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clean up the spacing here to be more even?

image

Since we use 4-space indentation in our code, we should try and use the same here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll normalize these before merging.

@rajadain rajadain assigned arottersman and unassigned rajadain Sep 7, 2017
@arottersman
Copy link
Author

Thanks for reviewing! I am also not sure where the api-docs url comes from...

* Write up an initial pass for the API views' documentation. Includes job
  descriptions as well as example requests, request types and formats, and
  example responses.

* NB: The example responses make the docstrings incredibly lengthy. While they
  appear nicely in Swagger under <details> elements, we may need to consider
  declaring them outside the views file
@arottersman
Copy link
Author

arottersman commented Sep 7, 2017

I've prettified the JSON in the docstrings. Will merge when the build completes.

@arottersman arottersman merged commit 958a01a into develop Sep 7, 2017
@arottersman arottersman deleted the arr/api-swagger branch September 7, 2017 17:39
@rajadain rajadain mentioned this pull request Oct 16, 2017
caseycesari added a commit that referenced this pull request Oct 25, 2017
The user namespace is added to the user URLs, which, because of our
Swagger configuration, prevents the URLs from showing up in the
documentation.

This was originally added in #2227, but it broke compatiability with
ITSI and was removed in #2377.

The namespace is readded here along with a fix to the ITSI login view.
When calling reverse on a URL which is included in a namespace, the
namespace must also be added to the reverse parameter. See
https://stackoverflow.com/a/38390178/217378.

Refs #2401
caseycesari added a commit that referenced this pull request Oct 25, 2017
The user namespace is added to the user URLs, which, because of our
Swagger configuration, prevents the URLs from showing up in the
documentation.

This was originally added in #2227, but it broke compatibility with
ITSI and was removed in #2377.

The namespace is added here again along with a fix to the ITSI login view.
When calling reverse on a URL which is included in a namespace, the
namespace must also be added to the reverse parameter. See
https://stackoverflow.com/a/38390178/217378.

Refs #2401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants