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

python 2 and 3, after merge with current master #437

Merged
merged 120 commits into from Nov 9, 2018

Conversation

Projects
None yet
8 participants
@orodeh
Copy link
Member

orodeh commented Nov 6, 2018

No description provided.

@orodeh orodeh requested a review from commandlinegirl Nov 6, 2018

@commandlinegirl
Copy link
Member

commandlinegirl left a comment

I think this large update would be easier to handle if some of these changes were committed separately, e.g. local argcomplete substitution, Makefile split, or packages upgrade (I understand it may have beed difficult as the number of changes grew).

@@ -1,4 +1,4 @@
#!/usr/bin/env python
#!/home/orodeh/dx-toolkit/build/py_env2.7/bin/python2

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

The shebang should not point to /home/orodeh

cmd = ['py.test', '-vv', '-s', '-m', 'TRACEABILITY_MATRIX', 'src/python/test/']

subproc_env = dict(os.environ)
#cmd = ['python', '-m', 'unittest']

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

Please, remove these comments.

@@ -648,8 +647,8 @@ def compose_local_dir(d, remote_folder, remote_subfolder):

# Downloading files
describe_input = dict(fields=dict(folder=True, name=True, id=True))
for remote_file in dxpy.search.find_data_objects(classname='file', state='closed', project=project,
folder=normalized_folder, recurse=True, describe=describe_input):
for remote_file in list(dxpy.search.find_data_objects(classname='file', state='closed', project=project,

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

is it necessary to convert it to a list instead of keeping a generator? How big a result can we expect?


if sys.version_info < (2, 7):
raise Exception("dxpy requires Python >= 2.7")

# Pypi is the repository for python packages.

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

Thanks for adding the comments!


_run_all_tests = 'DXTEST_FULL' in os.environ
TEST_AZURE = ((os.environ.get('DXTEST_AZURE', '').startswith('azure:') and os.environ['DXTEST_AZURE']) or
(os.environ.get('DXTEST_AZURE') and 'azure:westus'))
TEST_ISOLATED_ENV = _run_all_tests or 'DXTEST_ISOLATED_ENV' in os.environ
TEST_BENCHMARKS = 'DXTEST_BENCHMARKS' in os.environ ## Used to exclude benchmarks from normal runs

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

Thanks for ordering these variables!

@@ -322,11 +319,25 @@ def without_auth():
os.environ['DX_SECURITY_CONTEXT'] = prev_security_context


class DXTestCase(unittest.TestCase):
class DXTestCaseCompat(unittest.TestCase):
# mothod removed in python3

This comment has been minimized.

@commandlinegirl

commandlinegirl Nov 7, 2018

Member

nit: typo "mothod"

@commandlinegirl
Copy link
Member

commandlinegirl left a comment

Thanks, Ohad!

@orodeh orodeh merged commit 6c6c5a1 into master Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment