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

group resources by path #5

Merged
merged 3 commits into from
Jun 14, 2017
Merged

group resources by path #5

merged 3 commits into from
Jun 14, 2017

Conversation

woylie
Copy link
Member

@woylie woylie commented Jun 2, 2017

Addresses issue #2.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+0.1%) to 94.382% when pulling 24cbffd on group-by-path into af71c6d on master.

@woylie
Copy link
Member Author

woylie commented Jun 2, 2017

This fixes the aglio warnings. Aglio will now group the resources in the navigation bar like this:

aglio-example

I preferred it the way before, but I don't like the warnings.

Aglio will only show route parameters if they are defined within our macro. In the next screenshot, both actions are grouped under the same resource path /cats/{id}, but the id is only displayed in the first action.

aglio-example-2

@woylie
Copy link
Member Author

woylie commented Jun 2, 2017

It turns out that it is possible to define parameters directly under the resource path headers. For example:

## /cats/{id}

+ Parameters

    + id

### DELETE

You can put all parameters under the resource path headers as above, or you could place the route parameters under the resource path headers and the query parameters under the action headers, like this:

## /cats/{id}{?delivery-method}

+ Parameters

    + id

### DELETE

    + Response 204

### GET

    Parameters

        + delivery-method

    + Response 200

The generated action headers and example routes will then always contain the path parameters (as they should), but only include those query parameters that are included in the tests.

This would be very nice for the generated documentation, but we would have to determine how to get the parameter information of all the actions on a path. We'd have to define the resource path information separately from the api/3 macro.

@rhazdon
Copy link
Member

rhazdon commented Jun 2, 2017

Yes, this will be a challenge. But I think this will not be that difficult. We have to expand the logic for matching the documented routes with the "real" routes a little bit, like cutting the routes for parameter or allowing them.

@woylie
Copy link
Member Author

woylie commented Jun 3, 2017

I don't know what you mean with cutting the routes for parameter or allowing them. We don't have to do anything special with routes. The question is this: We define the parameters at the action level, but now we want to display some of them one step higher in the hierarchy. If we leave the macros as they are, we would have to pick one of actions within a resource section and use the parameter information used there. But we don't want to do that. So we would have to define the route parameters within a third macro just for the resource section. But I don't like that either for usability reasons.

Let's leave this pull request as it is for now and release 0.3.1 soon.

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.1%) to 94.382% when pulling 9e21ad5 on group-by-path into 4129768 on master.

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.1%) to 94.382% when pulling 03bc904 on group-by-path into 4129768 on master.

@woylie woylie merged commit 3f2fe3b into master Jun 14, 2017
@woylie woylie deleted the group-by-path branch June 14, 2017 11:47
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.

3 participants