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

sort out Repo API for individual commits & their diffs #63

Closed
wants to merge 1 commit into from

Conversation

cvrebert
Copy link
Contributor

X-Ref: #56.

I'm not entirely sure having both /commits/<thing>.diff and /commits/<thing> ever worked completely properly with Flask/Werkzeug's routing system. When I started unit testing, some gremlins exposed themselves.

Another alternative is to merge them into a single /commits/<almost-arbitrary-string> endpoint, and then have the view function figure out whether it should serve a diff or JSON, but:

  • that guessing logic is annoying to write
  • it seems difficult/impossible to have Flask make /commits/<thing>.diff (rather than /commits/<thing>.diff/) and /commits/<thing>/ (rather than /commits/<thing>) be the respective canonical URLs
  • and /commit/<thing>.diff is the only version that GitHub recognizes anyway

Therefore, this pull request implements the endpoints GitHub-style (i.e. /commits/<thing>/ and /commit/<thing>.diff). (Yes, their scheme is inconsistent in that all other related APIs are under /commits/*.)

CC: @rajivm @dlindquist @kainswor for review

(P.S.: @rajivm, You seem to have outdone GitHub by managing to support diff-ing the initial commit 😄 )

@cvrebert cvrebert closed this Dec 17, 2013
@cvrebert
Copy link
Contributor Author

Well, GitHub apparently auto-closed this PR for some reason, but anyway, I'm implementing a rebased version of this as I continue with my merging of develop into master.
Once the code is in place with the test suite passing, I'd be quite open to another PR that adds a working /commits/<refspec>.diff endpoint alias.

@cvrebert cvrebert deleted the commit-commits-diff-api branch December 20, 2013 23:53
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.

1 participant