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

Added Open In Collab Badge on top of Notebook #1

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

Conversation

gshanbhag525
Copy link

Added Open In Collab Badge on top of Notebook for easily running the notebook in google collab

@rahul-jha98
Copy link
Owner

Hey @gshanbhag525. Thanks for the PR, it would be a great addition to have a direct link in the notebook.
However, I would like to point out that the method you used to add link to colab is navigating to your forked branch's notebook. This would be not be ideal to merge as if in case you make a change in your forked branch as per your need in future, it would break the consistency in my branch since the code viewed on colab would not match the one on Github.

Therefore I would add the open in colab using my branch's Github link and commit it using colab. Hope you understand the issue in merging your PR.

@gshanbhag525
Copy link
Author

Ohh. I had thought the same. Is it ok if I update the PR and point it to your github link?

@rahul-jha98
Copy link
Owner

Okay sure.
Here are some of the changes you need to do. See, when contributing to open source always make sure that there is only single commit for one feature. Your PR consist of 4 commits. Make sure you squash the commits into single one.

Also, for adding a link to colab in one notebook takes only 8 lines addition. So, for two notebooks it would take 16 line addition only. Yet for some reason your PR consists of 2374 additions and 2312 deletions.

So, squash the commits to single commit and restrict the changes to minimum possible needed (16-20 lines I guess) and update this PR.

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.

2 participants