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

Examples; replace Uber with USPTO #1419

Merged
merged 1 commit into from Dec 4, 2017
Merged

Conversation

MikeRalphson
Copy link
Member

This PR replaces the commercial, outdated and oAuth protected Uber example with a public, current and open one from the US Patent & Trademark Office.

Disclaimer: This is a slightly cut-down (bulk download endpoints have been removed for brevity) converted OpenAPI 2.0 definition (thus the Uber example in v2.0 could also be replaced). The response model is not fully defined.

Although it relies on a US Government department, it seems unlikely to come under direct threat from the current administration.

@earth2marsh earth2marsh self-requested a review November 30, 2017 15:57
Copy link
Member

@earth2marsh earth2marsh left a comment

Choose a reason for hiding this comment

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

I totally dig this direction and am happy even to approve it as-is (note: I didn't run it through any validators, but assume that was done already). Nice work!

There is a fine balance to strike between a simple, approachable spec and one that is too complicated. However, it's worth considering whether any of the following can be added:

  • A generic example response, like 5xx
  • Verbs other than get, especially a post example. Uber didn't have those when it was written, and it's possible that the USPTO doesn't either. If that's the case, maybe it is worth adding a more complicated example.
  • There are no examples of reusable components. Again, that may be an intentional choice, just worth considering.

@@ -0,0 +1,216 @@
openapi: 3.0.0
servers:
- url: 'https://developer.uspto.gov/ibd-api'
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth turning the scheme into a variable with enums of http and https?
If not, what about adding a description to explain the two URLs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I must confess I hadn't thought of using server variables for anything except the host, port or path (where RFC6570 templates are normally used), but happy to go with that, or the description option.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong pref, either will improve this slightly. Given that http/https should be somewhat common, it seems like that is self-evident as a variable.

url: 'https://developer.uspto.gov'
email: developer@uspto.gov
tags:
- name: bulkdata
Copy link
Member

Choose a reason for hiding this comment

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

Consider a description for the tag to show best practices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will amend.

@MikeRalphson
Copy link
Member Author

I've replaced the Bulk Data Search API with the Data Set API, as this has a post request as well as get. Hopefully this addresses the other points raised:

  • uses server variable for scheme
  • includes tag descriptions
  • includes an example response
  • has $ref to #/components/schemas
  • has been validated & checked against both the draft schemas

Copy link
Member

@earth2marsh earth2marsh left a comment

Choose a reason for hiding this comment

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

Nice work @MikeRalphson ! We should consider this for the first patch release.

@darrelmiller
Copy link
Member

Thanks for all your work on this @MikeRalphson !

@darrelmiller darrelmiller merged commit ae2e404 into OAI:master Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants