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

Python3 module updates #64

Merged

Conversation

pferate
Copy link
Contributor

@pferate pferate commented Mar 4, 2015

Got the code to pass all of the tests in python 2.6, 2.7, 3.3, and 3.4.

Removed 1 questionable test and having another test bypassed if in python 3. (see 2 of the later commits for more info)

@pferate pferate mentioned this pull request Mar 4, 2015
@@ -712,7 +707,7 @@ def method(self, **kwargs):
raise TypeError('media_filename must be str or MediaUpload.')

# Check the maxSize
if maxSize > 0 and media_upload.size() > maxSize:
if media_upload.size() and media_upload.size() > maxSize > 0:

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@methane
Copy link
Contributor

methane commented Mar 4, 2015

LGTM. Great Job!

@pferate pferate mentioned this pull request Mar 4, 2015
@nathanielmanistaatgoogle
Copy link
Contributor

Oh, and in case my perspective in this code review comes across as weird and unfamiliar in some respects, it might be because I make a point of reviewing provenance (https://code.google.com/p/soc/wiki/ContributingCode#Provenance) as well as content and metadata. It's certainly what's behind my asking pull requests to be of "short" (only a few commits) branches, and recently branched off head-of-master.

Note how this pull request now stands at 18 commits, some of the later of which undo the changes done in earlier commits. That's the kind of mess that should get wiped away in a commit-squashing process prior to merge of a pull request.

@pferate
Copy link
Contributor Author

pferate commented Mar 4, 2015

Ok, thanks! I'm going through and cleaning up the commits now. Some of those commits that undo previous commits were put in after the initial PR was created, but they are getting cleaned up now.

@pferate pferate force-pushed the python3-module_updates branch from 7f6dfc0 to e0af753 Compare March 4, 2015 21:19
@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@pferate pferate force-pushed the python3-module_updates branch from e0af753 to 676caa7 Compare March 4, 2015 21:33
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@nathanielmanistaatgoogle nathanielmanistaatgoogle added the cla: yes This human has signed the Contributor License Agreement. label Mar 4, 2015
@pferate pferate force-pushed the python3-module_updates branch from 676caa7 to 3a999c3 Compare March 4, 2015 21:48
@pferate
Copy link
Contributor Author

pferate commented Mar 4, 2015

Alright.... I think we're good now. I've taken out the extra commits and cut it down to 11 commits. I had some fun with the rebasing, but I think I figured it out.

@pferate
Copy link
Contributor Author

pferate commented Mar 4, 2015

It looks like there is another issue that needs to be addressed. I think when I was developing on my Windows computer last night, the permissions got changed on a number of files and I didn't notice. It added the execute bit, which causes nose to skip the tests. I verified that all of the tests still pass.

From the nose documentation:

It is important to note that the default behavior of nose is to not include tests from files which are executable. To include tests from such files, remove their executable bit or use the –exe flag (see ‘Options’ section below).

Let me know if you want me to fix the permissions on this PR or update the tox.ini in another PR.

@nathanielmanistaatgoogle
Copy link
Contributor

Fix the permissions in this pull request, please. Files in source control should not have execute bits set accidentally; they should only have them set if it is the authorial intent of the project maintainers.

@methane methane mentioned this pull request Mar 5, 2015
@pferate
Copy link
Contributor Author

pferate commented Mar 5, 2015

So it looks like @unittest.skip() and @unittest.skipif() were added in 2.7, so the 2 tests fail as is in 2.6 (see the travis logs). It looks like we can use the package unittest2.

unittest2 is a backport of the new features added to the unittest testing framework in Python 2.7 and onwards. It is tested to run on Python 2.6, 2.7, 3.2, 3.3, 3.4 and pypy.

I know that it isn't specific to Python3, but the skipif() was introduced in this PR. There are a few ways to handle this, but how would you like it done?

@nathanielmanistaatgoogle
Copy link
Contributor

I think that what you've done in fde7488 looks like the right way to use unittest2 to handle 2.6 compatibility.

Looking forward: fbae355 and fde7488 shouldn't exist as they merely tweak work done earlier on the not-yet-merged-to-master line of development. I think you should simply amend those earlier commits to fix the issues in them.

Also on the matter of amending commits: please follow the guidelines at chris.beams.io/posts/git-commit/#seven-rules. We should be able to look at https://github.com/google/google-api-python-client/pull/64/commits and not see any subject lines cut off and prematurely ending in ellipses.

This feels like it's getting close! :-)

@pferate
Copy link
Contributor Author

pferate commented Mar 5, 2015

I've learned a lot about rewriting history these past few days. :)

fbae355 has been incorporated to previous commits. (It was interesting splitting it up and squashing to previous commits)

fde7488 was needed for the 2 tests that were modified in 749477c, but I did it for all files for consistancy. In my mind they could be seen as atomic and in separate commits, but maybe in reverse order.

I'll modify the commit messages and move fde7488 until I hear otherwise.

@pferate pferate force-pushed the python3-module_updates branch from fde7488 to 807ab80 Compare March 5, 2015 22:22
@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@nathanielmanistaatgoogle
Copy link
Contributor

Sounds like the right approach.

One more minor nitpick: "consistency", not "consistancy".

@pferate pferate force-pushed the python3-module_updates branch 2 times, most recently from 77b26ef to 9725ff5 Compare March 5, 2015 23:10
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@pferate pferate force-pushed the python3-module_updates branch 2 times, most recently from 7f2b718 to d8ed894 Compare March 5, 2015 23:18
@pferate
Copy link
Contributor Author

pferate commented Mar 5, 2015

Alright... I think I got it all. I broke the history for a bit, but I think I got everything back in it's place.

At least I was spelling it "consistancy" with consistency... :-p

nathanielmanistaatgoogle referenced this pull request in pferate/google-api-python-client Mar 5, 2015
For Python 2.6 compatibility in testing.
@pferate pferate force-pushed the python3-module_updates branch from d8ed894 to 359631e Compare March 6, 2015 15:21
@pferate
Copy link
Contributor Author

pferate commented Mar 6, 2015

@nathanielmanistaatgoogle: Alright, I've made the last few changes, how does it look now?

@nathanielmanistaatgoogle
Copy link
Contributor

Looks pretty good, but where is the separate pull request introducing unittest2? It may have been formed out of this but it should be merged and then this pull request should be rebased on top of it.

@pferate
Copy link
Contributor Author

pferate commented Mar 9, 2015

I created #66 to introduce unittest2. Once it gets merged, I'll rebase this branch to the merge commit.

@pferate pferate force-pushed the python3-module_updates branch from 359631e to 22fe5c7 Compare March 9, 2015 17:36
@pferate
Copy link
Contributor Author

pferate commented Mar 9, 2015

Rebased this branch to 4e7e6d4.

@nathanielmanistaatgoogle
Copy link
Contributor

Made it the whole way through, finally, so (at least in theory) I'm done asking questions about the change. :-)

@pferate pferate force-pushed the python3-module_updates branch from 22fe5c7 to 846befc Compare March 9, 2015 19:36
@pferate
Copy link
Contributor Author

pferate commented Mar 9, 2015

I updated commit ca397a7 to just check the current version of python and run the appropriate check.

@nathanielmanistaatgoogle
Copy link
Contributor

I think this is ready for merge.

@methane may I ask you to make another quick re-review since a few things have changed over the course of the conversation?

@methane
Copy link
Contributor

methane commented Mar 10, 2015

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants