Summary

Do basic python 3 modernization

Review Request #9643 — Created Feb. 16, 2018 and discarded Feb. 17, 2018, 8:18 a.m.

Information

Owner

solarmist

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Updated the code to be Python 3 compatible.
Also, cleaned up flake8 issues, removed outdated python idioms, import six only from the package level to make it easy to find and remove in the future, and style inconsistencies across all of the files as best I could.

Specifically, I cleaned up:

all logging messages that used % instead of commas.
Inconsistent use of quotes within files
PEP 8 issues
A couple of refactors to make code easier to understand (specifically encode_multipart_formdata)
Removed comments about bugs from Prior to 2.6 and removed special handling for those fixed bugs
Made import blocks more consistent (std lib import, std lib from, other lib import, other lib from, RBTools import, RBTools from)
Fixed all of the bare try/except blocks
etc.

Testing Done

Posted and updated with Python 3

./bin/flake8 *.py tests rbtools

Testing

./bin/nosetests-2.7 -v

Testing the cache with a high max-age value ... ok
Testing the cache with a zero max-age value ... ok
Testing the cache with the must-revalidate control ... ok
Testing the cache with the no-store control ... ok
Testing the cache with the no-cache control ... ok
Testing the cache with the no-cache control and a specified ETag ... ok
Testing the cache with the no-cache control and an updated ETag ... ok
Testing the cache with the no-cache control ... ok
Testing the cache with the no-cache control and an updated Last-Modified header ... ok
Testing the cache with the Expires header in the past ... ok
Testing the cache with the Expires header in the future ... ok
Testing the cache with an Expires header that is overridden ... ok
Testing the cache with the Pragma: no-cache header ... ok
Testing writing to the cache with non-ASCII data ... ok
Testing the cache with the Vary header ... ok
Testing the cache with the Vary header and different requests ... ok
Testing Capabilities.has_capability with supported capability ... ok
Testing Capabilities.has_capability with partial capability path ... ok
Testing Capabilities.has_capability with unknown capability ... ok
Testing the default values. ... ok
Testing the multipart form data generation. ... ok
Testing the encoding of multipart form data with unicode and binary field data ... ok
Testing constructing a count resource. ... ok
Testing constructing an item resource. ... ok
Testing constructing a list resource. ... ok
Testing constructing without guessing the resource token. ... ok
Testing constructing a resource with a specific base class. ... ok
Testing guessing the resource's token. ... ok
Testing rewriting of extra_data__ parameters to create ... ok
Testing rewriting of exta_data__ parameters to update ... ok
Testing item resource fields. ... ok
Testing item resource link generation. ... ok
Testing access of a link field. ... ok
Testing link resource link generation. ... ok
Testing list resource lists. ... ok
Testing access of a dictionary field. ... ok
Testing iterating sub-fields of a dictionary field. ... ok
Testing generation of methods for the root resource uri templates. ... ok
Testing ReviewRequestResource.absolute_url with 'absolute_url' field ... ok
Testing ReviewRequestResource.absolute_url with generated fallback URL ... ok
Testing ReviewRequestResource.absolute_url with 'url' field ... ok
Testing BazaarClient diff with file exclusion. ... ok
Testing BazaarClient diff with file exclusion in a subdirectory. ... ok
Testing BazaarClient diff with file exclusion in the repo root. ... ok
Testing BazaarClient diff with changes between a 2nd level descendant ... ok
Testing BazaarClient diff with changes only in the parent branch ... ok
Testing BazaarClient simple diff case ... ok
Testing BazaarClient simple diff with multiple commits case ... ok
Testing BazaarClient diff with specific files ... ok
Testing BazaarClient get_repository_info with child branch ... ok
Testing BazaarClient get_repository_info, no branch ... ok
Testing BazaarClient get_repository_info with original branch ... ok
Testing BazaarClient guessing summary and description ... ok
Testing BazaarClient guessing summary and description for grand parent branch. ... ok
Testing BazaarClient guessing summary and description with a revision range. ... ok
Testing BazaarClient.parse_revision_spec with no specified revisions ... ok
Testing BazaarClient.parse_revision_spec with one specified revision ... ok
Testing BazaarClient.parse_revision_spec with one specified revision and a parent diff ... ok
Testing BazaarClient.parse_revision_spec with R1..R2 syntax ... ok
Testing BazaarClient.parse_revision_spec with two revisions ... ok
Testing BazaarClient parse_revision_spec with too many revisions ... ok
Testing GitClient.create_commit with all_files set to True. ... ok
Testing GitClient.create_commit with run_editor set to True. ... ok
Testing GitClient.create_commit with all_files set to False. ... ok
Testing GitClient.create_commit with run_editor set to False. ... ok
Testing GitClient.delete_branch with merged_only set to True. ... ok
Testing GitClient.delete_branch with merged_only set to False. ... ok
Testing GitClient diff with divergent branches ... ok
Testing GitClient simple diff with file exclusion. ... ok
Testing GitClient simple diff with file exclusion in a subdir ... ok
Testing GitClient diff with file exclusion in the repo root. ... ok
Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote ... ok
Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch ... ok
Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch ... ok
Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch ... ok
Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path ... ok
Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote ... ok
Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch ... ok
Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches ... ok
Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote ... ok
Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote ... ok
Testing GitClient diff with a local tracking branch ... ok
Testing GitClient simple diff case ... ok
Testing GitClient simple diff with multiple commits case ... ok
Testing GitClient diff with tracking branch that has slash in its name ... ok
Testing GitClient diff with a tracking branch, but no origin remote ... ok
Testing GitClient diff with option override for tracking branch ... ok
Testing GitClient.get_raw_commit_message ... ok
Testing GitClient get_repository_info, simple case ... ok
Testing GitClient.merge with an invalid destination branch. ... ok
Testing GitClient.merge with an invalid target branch. ... ok
Testing GitClient.merge with squash set to True. ... ok
Testing GitClient.merge with squash set to False. ... ok
Testing GitClient.parse_revision_spec with no specified revisions ... ok
Testing GitClient.parse_revision_spec with no specified revisions and a parent diff ... ok
Testing GitClient.parse_revision_spec with one specified revision ... ok
Testing GitClient.parse_revision_spec with one specified revision and a parent diff ... ok
Testing GitClient.parse_revision_spec with diff-since-merge syntax ... ok
Testing GitClient.parse_revision_spec with diff-since syntax ... ok
Testing GitClient.parse_revision_spec with two specified revisions ... ok
Testing GitClient.push_upstream with 'git push' disabled. ... ok
Testing GitClient.push_upstream with an invalid remote branch. ... ok
Testing GitClient scan_for_server using repo property ... ok
Testing GitClient scan_for_server, .reviewboardrc case ... ok
Testing GitClient scan_for_server, simple case ... ok
Testing GitClient parse_revision_spec with too many revisions ... ok
Testing MercurialClient diff with diverged branch ... SKIP
Testing MercurialClient diff with file exclusion ... SKIP
Testing MercurialClient diff with empty file exclusion ... SKIP
Testing MercurialClient parent diffs with a diverged branch ... SKIP
Testing MercurialClient parent diffs with a simple case ... SKIP
Testing MercurialClient parent diffs with a diverged branch and --parent option ... SKIP
Testing MercurialClient diff, simple case ... SKIP
Testing MercurialClient diff with multiple commits ... SKIP
Testing MercurialClient get_repository_info, simple case ... SKIP
Testing MercurialClient guess summary & description 1 commit ... SKIP
Testing MercurialClient guess summary & description middle commit ... SKIP
Testing MercurialClient guess summary & description 3 commits ... SKIP
Testing MercurialClient guess summary & description 2 commits ... SKIP
Testing MercurialClient.parse_revision_spec with no arguments ... SKIP
Testing MercurialClient.parse_revision_spec with one revision ... SKIP
Testing MercurialClient.parse_revision_spec with r1::r2 syntax ... SKIP
Testing MercurialClient.parse_revision_spec with r1..r2 syntax ... SKIP
Testing MercurialClient.parse_revision_spec with parent base ... SKIP
Testing MercurialClient.parse_revision_spec with two revisions ... SKIP
Testing MercurialClient scan_for_server when in .reviewboardrc ... SKIP
Testing MercurialClient scan_for_server, simple case ... SKIP
Testing MercurialClient scan_for_server when present in hgrc ... SKIP
Testing MercurialClient (+svn) _calculate_hgsubversion_repository_info properly determines repository and base paths ... SKIP
Testing MercurialClient (+svn) diff specifying a revision ... SKIP
Testing MercurialClient (+svn) diff, simple case ... SKIP
Testing MercurialClient (+svn) diff with multiple commits ... SKIP
Testing MercurialClient (+svn) get_repository_info, simple case ... SKIP
Testing MercurialClient (+svn) scan_for_server in svn property ... SKIP
Testing MercurialClient (+svn) scan_for_server in .reviewboardrc ... SKIP
Testing MercurialClient (+svn) scan_for_server, simple case ... SKIP
Testing P4Wrapper.counters ... ok
Testing P4Wrapper.info ... ok
Testing PerforceClient.normalize_exclude_patterns ... ok
Testing PerforceClient.diff with a submitted changelist ... ok
Testing PerforceClient.diff with moved files and capability off ... ok
Testing PerforceClient.diff with moved files and capability on ... ok
Testing PerforceClient.diff with a pending changelist ... ok
Testing PerforceClient.parse_revision_spec with invalid specifications ... ok
Testing PerforceClient.parse_revision_spec with no specified revisions ... ok
Testing PerforceClient.parse_revision_spec with a pending changelist ... ok
Testing PerforceClient.parse_revision_spec with a shelved changelist ... ok
Testing PerforceClient.parse_revision_spec with a submitted changelist ... ok
Testing PerforceClient.parse_revision_spec with two changelists ... ok
Testing PerforceClient.get_repository_info ... ok
Testing PerforceClient.get_repository_info outside client root ... ok
Testing PerforceClient.scan_for_server_counter with reviewboard.url ... ok
Testing PerforceClient.scan_for_server_counter with encoded reviewboard.url.http:|| ... ok
Testing SVNClient diff with file exclude patterns ... ok
Testing SVNClient diff with exclude patterns in a subdir ... ok
Testing SVNClient diff with repo exclude patterns in a subdir ... ok
Testing SVNClient diff with a non-utf8 file ... ok
Testing SVNClient diff with a non-utf8 filename via repository_url option ... ok
Testing SVNClient.get_commit_message with a single committed revision ... ok
Testing SVNClient.get_commit_message with multiple committed revisions ... ok
Testing SVNClient.get_commit_message with a working copy change ... ok
Testing SVNClient.history_scheduled_with_commit nominal cases ... ok
Testing SVNClient.history_scheduled_with_commit with exclude file ... ok
Testing SVNClient.history_scheduled_with_commit is bypassed when diff is not for local modifications in a working copy ... ok
Testing SVNClient.parse_revision_spec with invalid specifications ... ok
Testing SVNClient.parse_revision_spec with no specified revisions ... ok
Testing SVNClient.parse_revision_spec with a non-utf8 log entry ... ok
Testing SVNClient.parse_revision_spec with R1:R2 syntax ... ok
Testing SVNClient.parse_revision_spec with one specified numeric revision ... ok
Testing SVNClient.parse_revision_spec with one specified changelist revision ... ok
Testing SVNClient.parse_revision_spec with one specified invalid changelist revision ... ok
Testing SVNClient.parse_revision_spec with one revision and a repository URL ... ok
Testing SVNClient.parse_revision_spec with two revisions ... ok
Testing SVNClient.parse_revision_spec with R1:R2 syntax and a repository URL ... ok
Test diff with removal of lines that look like headers ... ok
Testing SVNClient identical diff generated from root, subdirectory, and via target ... ok
Testing SVNClient with --show-copies-as-adds functionality disabled ... ok
Testing SVNClient with --show-copies-as-adds functionality enabled ... ok
Testing SVNRepositoryInfo.find_server_repository_info with mirror_path matching ... ok
Testing SVNRepositoryInfo.find_server_repository_info with path matching ... ok
Testing SVNRepositoryInfo.find_server_repository_info with UUID matching ... ok
Testing SVNRepositoryInfo._get_relative_path ... ok
Testing argument duplication with rbt post --field myField=test --description test ... ok
Testing multiple delimiters with rbt post --field myField=this=string=has=equals=signs ... ok
Testing built-in fields through extra_fields with rbt post --field description=testing --field summary='native testing' --field testing-done='No tests' ... ok
Testing setting target group via config file with rbt post ... ok
Testing setting target person via config file with rbt post ... ok
Testing multiple extra field arguments with rbt post --field foo=bar --field desc=new ... ok
Testing setting target group on update via config file with rbt post --review-request-id=12345 ... ok
Testing setting target person on update via config file with rbt post --review-request-id=12345 ... ok
Testing one extra field argument with rbt post --field foo=bar ... ok
Testing setting the target group with rbt post --target-groups=test_group ... ok
Testing setting the target group on an update with rbt post --target-groups=test_group --review-request-id=12345 ... ok
Testing setting the target person with rbt post --target-people=test_person ... ok
Testing setting the target person on an update with rbt post --target-people=test_person --review-request-id=12345 ... ok
Testing built-in fields through extra_fields with rbt post --field description and rbt post --field testing_done='No tests' ... ok
Testing variable substitution with alphabetic variables ... ok
Testing alias substitution with a slash at the end of the string ... ok
Testing variable substitution where no variables are supplied ... ok
Testing alias substitution with bad quotes. ... ok
Testing variable substitution in rbtools aliases ... ok
Testing variable substitution where the argument isn't supplied ... ok
Testing variable substitution with the $ variable ... ok
Testing $ variable substitution with whitespace-containing args ... ok
Testing variable substitution where multiple variables appear ... ok
Testing 'check_install' method. ... ok
Testing 'execute' method. ... ok
Testing 'is_valid_version' method. ... ok
Testing 'make_empty_files' method. ... ok
Testing 'make_tempfile' method. ... ok

Ran 210 tests in 99.386s

OK (SKIP=30)

./bin/nosetests-3.4 -v

Ran 210 tests in 99.002s

OK (SKIP=30)

Issues

Description	From	Last Updated
Is this a squashed version of all the other commits? If so, you can remove it. We want every single …	chipx86	Feb. 17, 2018, 8:19 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Removed old style class(es) delcarations.

Description:

		Updated the code to be Python 3 compatible.

~		Also cleaned up flake8 error and style inconsistencies across all of the files.
	~	Also, cleaned up flake8 issues, removed outdated python idioms, import six only from the package level to make it easy to find and remove in the future, and style inconsistencies across all of the files as best I could.
		Specifically, I cleaned up:

		all logging messages that used % instead of commas.
		Inconsistent use of quotes within files
		PEP 8 issues
		A couple of refactors to make code easier to understand (specifically encode_multipart_formdata)
		Removed comments about bugs from Prior to 2.6 and removed special handling for those fixed bugs
		Made import blocks more consistent (std lib import, std lib from, other lib import, other lib from, RBTools import, RBTools from)
		Fixed all of the bare try/except blocks
		etc.

Commit:

ef481edc3676b9bf5be52c2ba9f24b28419ab50f

1f8e5261d4f4984d0526cf8b39eaf452e6fa4420

Diff:

Revision 2 (+778 -742)

Show changes

	README.md
	setup.py
	rbtools/api/cache.py
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/errors.py
	rbtools/api/factory.py
	rbtools/api/request.py
	61 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This change is basically un-reviewable as-is. It does a ton of different things, and the diff is huge. If you can break up your changes into individual changes that each accomplish one thing, we can take a look at them.

solarmist Feb. 16, 2018, 2:37 p.m.

Yup, that's my next task. I just wanted to put this up to share it.

solarmist Feb. 16, 2018, 8:26 p.m.

Broke it into 8 reviews.
https://reviews.reviewboard.org/r/9648/
https://reviews.reviewboard.org/r/9649/
https://reviews.reviewboard.org/r/9650/
https://reviews.reviewboard.org/r/9651/
https://reviews.reviewboard.org/r/9652/
https://reviews.reviewboard.org/r/9653/
https://reviews.reviewboard.org/r/9654/
https://reviews.reviewboard.org/r/9655/

The issue has been dropped. Show all issues

Is this a squashed version of all the other commits? If so, you can remove it. We want every single review request to be independent, landable on its own, and having a squashed version of all those will make things confusing.

Status:: Discarded