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

Forking #50

Merged
merged 2 commits into from
Jul 14, 2017
Merged

Conversation

mikekavouras
Copy link
Contributor

@mikekavouras mikekavouras commented Jul 14, 2017

Introducing forking 🍴 (closes #41)

forking

Behavior

  • The "fork" option will always be present on someone else's repo.
  • The repo will be forked to a user's personal account (can be expanded to support a user's organizations in another PR)

Notes (of disappointment)

GitHub API does not provide enough information to infer whether or not a user has forked a repo based on some origin repo.

Attempt 1

Repository Search
https://api.github.com/search/repositories?q=react-native%20user%3Amikekavouras%20fork%3Aonly%20in%3Aname

The response object does not contain any reference to the origin repo, so there's no way to tell exactly where this repository began.

Attempt 2

User Repositories
https://api.github.com/repos/${username}/${repo.name}

This solution assumes that the repo names are the same, which is an inaccurate assumption. If a user has a conflicting repo name, GitHub will automatically append a -n to the forked repo (e.g. react-native-1).

If there is something I overlooked, please feel free to collaborate.

One more thing...

Issue #3 is more pronounced with this PR because of navigation from repo screen to repo screen. Navigating back will not reload the previous repository state. This is outside the scope of this PR.

@mikekavouras mikekavouras force-pushed the mikekavouras/fork-repos branch 2 times, most recently from 4cfb263 to 148b315 Compare July 14, 2017 02:35
@@ -266,6 +302,16 @@ const styles = StyleSheet.create({
listTitle: {
color: colors.black,
fontFamily: 'AvenirNext-Medium'
},
overlay: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this isn't being used at all, most likely forgot to remove it when you created a separate component :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. removed.

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic! Aside from one tiny nit, everything looks good to me code-wise 👍

For Issue #3, definitely out of the scope of this PR!

Also a quick question with regards to Attempts 1 & 2, is there no simple way to know whether a user has already forked an existent repository? I'm assuming /repos/:owner/:repo/forks is the only actual way and it wouldn't be feasible because we can't apply search to it right?

No worries if that's the case. Functionally, if we've already forked a repo and attempt to fork it again - does the call pass and the user is navigated to the repository normally? If that's how it works, I'm okay with that - definitely acceptable with what we have :)

@mikekavouras
Copy link
Contributor Author

"is there no simple way to know whether a user has already forked an existent repository?"

As far as I can tell, the only reliable way to know if a user fork exists is to use /repos/:owner/:repo/forks. However, a repo like react-native has 11k+ forks meaning 350+ paged responses. Very impractical. The best idea I can come up with is some idea of, "where owner is ${user}". Something like /repos/:owner/:repo/forks?owner=username. Sadly, nothing like that exists.

"- does the call pass and the user is navigated to the repository normally?"

Yes it does. GH essentially ignores the request and sends us back the already forked repo JSON. The server response is the same in both cases and the resolution on the front end is consistent.

@housseindjirdeh
Copy link
Member

housseindjirdeh commented Jul 14, 2017

Something like /repos/:owner/:repo/forks?owner=username. Sadly, nothing like that exists.

Yeah that's exactly what I was hoping for, but it is what it is :)

Yes it does. GH essentially ignores the request and sends us back the already forked repo JSON. The server response is the same in both cases and the resolution on the front end is consistent.

Beautiful 👍

Cheers thank you so much mate, I'm so sorry I just merged another PR and there's a conflict in the README with the contributor section. If you can resolve that I'll merge this right in 🎉

@mikekavouras
Copy link
Contributor Author

@housseindjirdeh should be 👍 to merge

@housseindjirdeh
Copy link
Member

🎉 🎉 🎉 🎉 🎉

@housseindjirdeh housseindjirdeh merged commit 12df3ad into gitpoint:master Jul 14, 2017
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.

Add forking
2 participants