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

Adds API Blueprint #313

Merged
merged 6 commits into from
Jun 6, 2017
Merged

Adds API Blueprint #313

merged 6 commits into from
Jun 6, 2017

Conversation

kurko
Copy link
Contributor

@kurko kurko commented Nov 16, 2016

This commit adds API Blueprint (APIB) to this gem. A few highlights:

  • APIB groups entities, resources, routes, HTTP verbs and requests
    differently than what the gem currently uses. Because of that I had to
    override more methods than usual in the Markup classes.

    APIB has the following structure:

    1. resource (e.g "Group Orders")
    2. route (e.g "Orders Collection [/orders]")
    3. HTTP method (e.g "Loads all orders [GET]")
    4. Requests. Here, we show all different requests which means that
      we don't have the repetition of GET for each example. All
      examples stay under one, unified HTTP method header.
  • APIB differentiates parameters (values used in the URLs) from
    attributes (values used in the actual request body). You can use
    attributes in your texts.

  • APIB has a route header, which means we had to add a new RSpec block
    called route() which wraps HTTP methods, like the following:

    route "/orders", "Orders Collection" do
      get "Returns all orders" do
        # ...
      end
    
      delete "Deletes all orders" do
        # ...
      end
    end

    If you don't use route, then param in get(param) should be an URL.

  • APIB is not navigable like HTML, so generating an index file makes no
    sense. Because of that, we are generating just one file, index.apib.

  • We are omitting some APIB features in this version so we can get up
    and running sooner. Examples are object grouping, arrays objects and a
    few description points.

Unrelated to APIB:

  • FakeFS was being used globally in test mode, which means that
    nothing would load file systems, not even a simple ~/.pry_history, which
    made debugging impossible. I moved the usage of this gem to the places
    where it is used.

Closes #235.

@kurko
Copy link
Contributor Author

kurko commented Nov 16, 2016

Don't be scared with the +1,023 −52 there. It's mostly the template and features/ files (cucumber has the entire expectation file in it).

@kurko kurko force-pushed the 235-blueprint-api branch 2 times, most recently from 683874e to 33a6dcb Compare November 16, 2016 20:27
@kurko
Copy link
Contributor Author

kurko commented Dec 7, 2016

A few issues when trying this out in my project (will keep this list up to date as I go):

  • Request without a defined Content-Type is generating + Request title () (parenthesis should be absent when content-type is not defined) (2f5cceb)
  • parameter :id, required: true renders id: (required, ) - id (c62e3a3)
  • parameter :id, required: false renders id: () - id (c62e3a3)
  • /users/:id{?scope=:scope} does not strip the curly braces, causing bad URI(is not URI?): /v1/users/beed46f4-7724-4318-8bae-16f192d49a4a{?application_id=:application_id} (ca98bb0)
  • When Content-Type: is not specifically application/json (e.g application/vnd.api+json), the response body doesn't get formatted to multiline. It stays all in one line.
  • Request/response are showing charset, not just media types (e.g + Response 422 (application/vnd.api+json; charset=utf-8))

kurko added a commit that referenced this pull request Dec 9, 2016
kurko added a commit that referenced this pull request Dec 9, 2016
* `parameter :id, required: true` renders `id: (required, ) -  id`
* `parameter :id, required: false` renders `id: () -  id`

Connects to #313
This commit adds API Blueprint (APIB) to this gem. A few highlights:

* APIB groups entities, resources, routes, HTTP verbs and requests
  differently than what the gem currently uses. Because of that I had to
  override more methods than usual in the `Markup` classes.

  APIB has the following structure:
    1. resource (e.g "Group Orders")
    2. route (e.g "Orders Collection [/orders]")
    3. HTTP method (e.g "Loads all orders [GET]")
    4. Requests. Here, we show all different requests which means that
       we don't have the repetition of GET for each example. All
       examples stay under one, unified HTTP method header.
* APIB differentiates parameters (values used in the URLs) from
  attributes (values used in the actual request body). You can use
  `attributes` in your texts.
* APIB has a `route` header, which means we had to add a new RSpec block
  called `route()` which wraps HTTP methods, like the following:

  ```ruby
  route "/orders", "Orders Collection" do
    get "Returns all orders" do
      # ...
    end

    delete "Deletes all orders" do
      # ...
    end
  end
  ```

  If you don't use `route`, then param in `get(param)` should be an URL.

* APIB is not navigable like HTML, so generating an index file makes no
  sense. Because of that, we are generating just one file, `index.apib`.
* We are omitting some APIB features in this version so we can get up
  and running sooner. Examples are object grouping, arrays objects and a
  few description points.

Unrelated to APIB:

* FakeFS was being used _globally_ in test mode, which means that
  nothing would load file systems, not even a simple `~/.pry_history`, which
  made debugging impossible. I moved the usage of this gem to the places
  where it is used.

Closes #235.
* `parameter :id, required: true` renders `id: (required, ) -  id`
* `parameter :id, required: false` renders `id: () -  id`

Connects to #313
This allows for routes to be defined with optional querystrings, like:

```ruby
route '/orders/:id{?optional=:optional}', "Single Order" do
```

Before this, the http methods (e.g `get`, `post`) were injecting the
optional into the final URI. This moves optionals into another metadata
key (`options[:route_optionals]`).
JSON requests should use UTF-8 by default according to
http://www.ietf.org/rfc/rfc4627.txt, so we will remove `charset=utf-8`
when we find it to avoid redundancy.

In the process, I moved code that was only used by APIB into the APIB
classes, such as exposing `content_type` to the templates. If I changed
the `content-type` for all templates it would break unrelated things.

Connects to #235.
@kurko
Copy link
Contributor Author

kurko commented Jan 11, 2017

Extra bugs:

  • explanation not working at route level (only at resource level)
  • explanation not working at verb level (only at resource level)

@jmcarp
Copy link

jmcarp commented Feb 19, 2017

This would be a great feature @kurko ! Is it still in progress?

@kurko
Copy link
Contributor Author

kurko commented Feb 21, 2017

@jmcarp I've been developing with it in the past 2 months here and it's working great. As I need more features, I realize I missed a few things here and there, for example the last checklist above (about explanation). I'll give it another month before I push it to be merged, but you can start using it locally just fine, it's pretty good right now. Your feedback would be appreciated.

@terry90
Copy link
Contributor

terry90 commented Apr 25, 2017

Can't wait to see it live ! Do you have an ETA ? ❤️

@kurko
Copy link
Contributor Author

kurko commented Apr 25, 2017

No ETA just yet, @terry90. We're using it internally in a multiple systems. We want to have it close to 100% when we merge it. As noted above in my last comment, explanation is pending. I have not found the time to work on that yet.

EDIT: if anyone would like to help with the explanation part, you're welcome.

@kurko
Copy link
Contributor Author

kurko commented May 23, 2017

@oestrich I think we're good to merge this one. We're using this in 7+ apps here and it's working pretty good so far cc @jakehow

Re: the 2 items at #313 (comment), I'll create a separate issue because they're not really bugs, just missing features.

@jakehow
Copy link
Member

jakehow commented Jun 6, 2017

@oestrich let me know if any concerns on getting this merged. We are using this successfully and I am 👍 on the PR from a code review standpoint.

@oestrich
Copy link
Contributor

oestrich commented Jun 6, 2017

My only concern is adding a DSL specific to a format that anything can use, but it's not a big one. Merging. Thanks for the PR @kurko

@oestrich oestrich merged commit 5a1130c into master Jun 6, 2017
@oestrich oestrich deleted the 235-blueprint-api branch June 6, 2017 16:16
@oestrich
Copy link
Contributor

oestrich commented Jun 6, 2017

5.0.0 is out including this.

@kurko
Copy link
Contributor Author

kurko commented Jun 6, 2017

@oestrich fantastic.

About the DSL, I share you concern. APIB has a different format, though, that requires some specific inputs. We just need to keep an eye to not grow beyond this. Perhaps it'd be a good idea to have adapters (via composition), not just inheritance, so we can extend beyond these formats without changing the core DSL.

Thanks for merging.

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.

5 participants