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.

Sign up for GitHub

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

Jump to bottom

Fix for issue #25 - Add support for branches with '/' in them #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix for issue #25 - Add support for branches with '/' in them #72

wants to merge 2 commits into from

Conversation

matt-watson90
Copy link
Contributor

This pull request updates github.js to use the branches API to split the filepath from the branch path and use them both accordingly in calls to /contents and /commits. This means we don't have to naively try and pull the sha variable from the path anymore.

@matt-watson90
Fix for pomber#25 - Support branches with '/' in them
bdc09dd 
Use the branches API to figure out all the branches present in the repo
and then split the path in to branch and fileName. The contents API
needs the branchname in the 'ref' field and the commits api uses it in
the 'sha' field.
@matt-watson90
Run prettier on files changed during fix for pomber#25
fad87ad
@matt-watson90
Copy link
Contributor Author

@pomber It looks like the build is failing because of an existing problem in master with prettier and ReadMe.md

@pomber
Copy link
Owner

pomber commented Feb 11, 2019

Thanks @matt-watson90 .
I see two problems with this approach:

  1. the sha from the url is not always a branch, take for example this:
    https://github.githistory.xyz/pomber/git-history/blob/941a28ad39703bd96b3b46a87f8a7d0e253affb5/package.json
    https://deploy-preview-72--github-history.netlify.com/pomber/git-history/blob/941a28ad39703bd96b3b46a87f8a7d0e253affb5/package.json
    It works with the current version but not with the PR version.

  2. we are adding one more request to the critical path, for something that is an edge case

Maybe we could try your approach after there is a 404. Keep the current approach, if the commits request respond with a 404, get the list of branches and see if we've got the sha/path wrong and try again.

What do you think?

@matt-watson90
Copy link
Contributor Author

I hadn’t considered that first problem, that’s a good spot.

I agree with your solution. I’ll see what I can implement tonight (or maybe tomorrow)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beezelbutt beezelbutt beezelbutt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@matt-watson90 @pomber @beezelbutt