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

API-5: documentation and specs #576

Closed

Conversation

wvengen
Copy link
Member

@wvengen wvengen commented Oct 15, 2018

Part five of #429 in chunks, continued from #573:

  • API documentation
  • Specs
  • Fix for Doorkeeper 5 compatibility (came up working on the specs)

To render the swagger file, see here.

@wvengen wvengen added the api label Oct 15, 2018
@wvengen wvengen added this to the API v1 milestone Oct 15, 2018
@wvengen wvengen force-pushed the feature/api-5-specs-and-docs branch from 9b0f564 to 947e6c1 Compare October 15, 2018 11:01
@wvengen wvengen force-pushed the feature/api-5-specs-and-docs branch from 947e6c1 to bea731c Compare October 15, 2018 11:04
@paroga
Copy link
Member

paroga commented Oct 15, 2018

i'd like to have the changes on the existing test in it's own commit, add the tests in the same commit as the controllers. why didn't you add the required column not in the first commit?

@wvengen
Copy link
Member Author

wvengen commented Oct 15, 2018

I could add the specs in separate commits if you like. I agree that would have been nicer. When writing, it felt like an additional feature (docs, then specs based on the docs), but you're right. I'll refactor it and create a separate PR / add it to the existing PRs.

I only discovered about the missing column when some condition in specs were hit, which didn't happen in the original PR but because of splitting I ended up with a newer version of Doorkeeper (which is good, but if I'd realized, I'd tested that better). Somehow the missing column didn't matter when using the API.

@paroga
Copy link
Member

paroga commented Oct 15, 2018

i'd go for updating to Doorkeeper5 first and then rebasing all other stuff on top of it

@wvengen
Copy link
Member Author

wvengen commented Oct 15, 2018

In hindsight I was too fast merging the earlier PRs ...

@wvengen
Copy link
Member Author

wvengen commented Oct 15, 2018

Setup and basic endpoints in #577, updated #572. Will update #573 after feedback.
That's much better, thanks for pointing that out!

@wvengen wvengen closed this Oct 15, 2018
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.

2 participants