Skip to content

Conversation

@seetea
Copy link
Contributor

@seetea seetea commented Mar 24, 2016

Fix issue #21 to make the example can be built under both python 2.7 and python 3.4.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove space before '('

@kirilg
Copy link
Contributor

kirilg commented Mar 24, 2016

Looks good overall, just one minor comment. In addition to that, please sign the CLA (instructions given by googlebot) and squash all commits after you apply the suggested change. Thanks.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@seetea
Copy link
Contributor Author

seetea commented Mar 26, 2016

Kirilg, thank you for your suggestions very much. But I'm sorry that I'm still waiting for my company's permission to sign the CLA. Could you please wait for a few days?

Anyway, I think I can make the commits meet your suggestions first.

I have squashed all the commits into the Fix issue #21,support both python 2.7 and python 3.4. Do you think it's OK now?

@kirilg
Copy link
Contributor

kirilg commented Mar 28, 2016

No problem. Please take a look at https://github.com/tensorflow/serving/blob/master/CONTRIBUTING.md (namely the part that says "If you work for a company that wants to allow you to contribute your work, then you'll need to sign a corporate CLA." along with a link to the corporate CLA page, which I'm guessing is what you did already). Just ping back on this PR once the licensing is sorted out, and I'll merge.

With regards to the commits, I still see multiple commits in the "commits" tab (8). Please squash so there's only one.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@seetea
Copy link
Contributor Author

seetea commented Mar 29, 2016

Kirilg, I think I have made some mistakes when I squashed the commits. The PR was closed and recreated at #26. Maybe we can continue the discussion there. Thanks.

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.

4 participants