minimal tweaks to update for python3 #83
Conversation
|
hmm, I'm not able to follow the link to see the failure, so I'm not sure how to debug this. |
|
Hmm, not sure why @vsoch Here's what's wrong: |
test/testdata/.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| .pyc | |||
There was a problem hiding this comment.
you can move this to the main gitignore
There was a problem hiding this comment.
I'll remove them here! I seem to remember I added them because a test failed that wanted to see them there.
|
@vsoch please also bump the version in |
|
one failure left (i'm clueless why these tests are not public anymore) |
|
hey @stdweird thanks for the quick feedback! Could you give me some hint about what the failing test is for? I was able to create a pyhon 2 environment and reproduce the error. I'm attaching the content of "txt" that is failing, and can be fixed by updating the search to not require the args to be at the beginning of the line (python 2 will have it there, python 3 will have an opening parens). This fixes that issue: But then for the next bug that comes up (this similar expression) it cannot match because of a change of ordering of the dictionary keys. The test expects "name" to be first: But the actual output doesn't have this. What are the general purposes of these tests? I think we should either rewrite to be more fitting to that, or just remove if there isn't a good use. In the meantime, I've pushed the other changes from the comments above! |
|
@vsoch i can have a look, but might not be before next monday... |
|
Totally no worries about this! I'll keep chewing on the other PR :) Thanks again for your speedy response! |
|
One more adjustment here as I'm testing vsc-base - there seems to be an issue with running the tests, here: as a (bad) workaround that is likely missing the upstream error, I'm just catching and returning None: specifically the error (in vsc-base) is the following: but it's obviously resolved by just skipping it. We can probably discuss why this happens and come up with a better strategy - this is a patch fix for now! |
test/shared_setup.py
Outdated
| package_list = list(setup.SHARED_TARGET['packages']) | ||
| package_list.sort() | ||
|
|
||
| self.assertEqual(package_list, ['vsc', 'vsc.test']) |
There was a problem hiding this comment.
Does this also works in Python 3?
self.assertEqual(sorted(setup.SHARED_TARGET['packages']), ['vsc', 'vsc.test'])
test/shared_setup.py
Outdated
| package_list = list(setup.SHARED_TARGET['packages']) | ||
| package_list.sort() | ||
|
|
||
| self.assertEqual(package_list, ['vsc', 'vsc.test']) |
There was a problem hiding this comment.
Same here, more straightforward (imho) change:
self.assertEqual(sorted(setup.SHARED_TARGET['packages']), ['vsc', 'vsc.test'])There was a problem hiding this comment.
I was mainly doing for readability, glad to change!
lib/vsc/install/headers.py
Outdated
| @@ -93,7 +93,8 @@ def get_header(filename, script=False): | |||
| if not os.path.isfile(filename): | |||
| raise Exception('get_header filename %s not found' % filename) | |||
|
|
|||
| txt = open(filename).read() | |||
| with open(filename) as filey: | |||
There was a problem hiding this comment.
@vsoch Maybe use fh everywhere rather than filey for consistency?
There was a problem hiding this comment.
filey is my special pet name for files since the dawn of time! Mostly because I hadn't a clue what else to use. We had a discussion, and filey and I are ok with fh too :)
lib/vsc/install/headers.py
Outdated
| @@ -226,7 +226,8 @@ def check_header(filename, script=False, write=False): | |||
|
|
|||
| if write and changed: | |||
| log.info('write enabled and different header. Going to modify file %s' % filename) | |||
| wholetext = open(filename).read() | |||
| with open(filename) as filey: | |||
lib/vsc/install/shared_setup.py
Outdated
| @@ -148,7 +160,7 @@ def _log(self, level, msg, args): | |||
|
|
|||
| RELOAD_VSC_MODS = False | |||
|
|
|||
| VERSION = '0.10.32' | |||
| VERSION = '0.10.33' | |||
There was a problem hiding this comment.
Maybe this should be a more significant version bump, e.g. 0.11.0?
test/headers.py
Outdated
| @@ -111,8 +111,8 @@ def test_gen_license_header(self): | |||
| } | |||
| for license in KNOWN_LICENSES.keys(): | |||
| res_fn = os.path.join(self.setup.REPO_TEST_DIR, 'headers', license) | |||
| result = open(res_fn).read() | |||
|
|
|||
| with open(res_fn) as filey: | |||
test/shared_setup.py
Outdated
| @@ -111,7 +111,7 @@ def test_rel_gitignore(self): | |||
| try: | |||
| self.setup.rel_gitignore(['testdata'], base_dir=base_dir) | |||
| except Exception as e: | |||
| self.assertTrue('.pyc' in e.message, msg=e) | |||
| self.assertTrue('.pyc' in str(e)) | |||
There was a problem hiding this comment.
please rename e to exc or err while you're at it (we shouldn't be using single-letter variables outside of list comprehensions)
test/shared_setup.py
Outdated
| print 'args: ', args | ||
| print 'kwargs:', kwargs | ||
| print('args: ', args) | ||
| print('kwargs:', kwargs) |
There was a problem hiding this comment.
This means trouble in Python 2 I think, and it may be why the test for this is failing in Python 3!
Should be this, to avoid that you're printing a tuple:
print('args: %s' % args)
print('kwargs: %s' % kwargs)
There was a problem hiding this comment.
if you put this at the top of the file from __future__ import print_function then print() will behave the same in python2 as in python3
test/shared_setup.py
Outdated
|
|
||
| self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M)) | ||
| self.assertTrue(re.search(r"^kwargs:\s*\{.*'name':\s*'vsc-test'", txt, re.M)) | ||
| # self.assertTrue(re.search(r"kwargs:.*\{.*'name':.*'vsc-test'", txt, re.M)) |
There was a problem hiding this comment.
I don't think you'll need to change this anymore if you keep the remark above for fake_setup into account...
test/shared_setup.py
Outdated
|
|
||
| self.mock_stdout(True) | ||
| action_target({'name': 'vsc-test', 'version': '1.0.0'}, setupfn=fake_setup, urltemplate='http://example.com/%(name)s') | ||
| txt = self.get_stdout() | ||
| self.mock_stdout(False) | ||
| self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M)) | ||
| self.assertTrue(re.search(r"args:.*\(\)", txt, re.M)) |
There was a problem hiding this comment.
Same here, no need to change this if you fix fake_setup above?
|
When I run with python3 I get a TON of these messages: return tokenize.open(filepath).read()
/home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/.eggs/prospector-0.12.7-py3.5.egg/prospector/encoding.py:32: ResourceWarning: unclosed file <_io.TextIOWrapper name='/home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/.eggs/pylint-1.8.4-py3.5.egg/pylint/__pkginfo__.py' mode='r' encoding='utf-8'>Any idea of why this happens (it seems to be from pylint?) and how we can avoid /address? It makes debugging weird because it's like you get hit by a tidal wave first! |
|
@vsoch Looks like that's an issue with |
|
ah okay, I'm going to try getting rid of prospector. |
|
@vsoch I don't think that's the proper way forward here, |
|
If the module is not developed to support python 3.5+, then it should not be used in the case of 3.5+. It can be again given life when it does have this support. |
lib/vsc/install/shared_setup.py
Outdated
| @@ -1254,13 +1275,17 @@ def parse_target(self, target, urltemplate=None): | |||
|
|
|||
| # update the cmdclass with ones from vsc_setup_klass | |||
| # cannot do this in one go, when SHARED_TARGET is defined, vsc_setup doesn't exist yet | |||
| for name, klass in new_target['cmdclass'].items(): | |||
| keepers = new_target.copy() | |||
| for name in list(new_target['cmdclass']): | |||
There was a problem hiding this comment.
@vsoch The list is no longer necessary now that you're using keepers, is it?
There was a problem hiding this comment.
I think it is - we have to iterate through something (it can't be keepers if we are making changes) and creating a list in this fashion is the way to have support for python2 and 3.
There was a problem hiding this comment.
But of course there are likely other solutions that would work here if you have preference!
There was a problem hiding this comment.
I still think it's fine to simply drop the list part...
We're iterating of the keys of a dictionary here, which we are not modifying, that works the same in both Python 2 & 3? Or am I missing something?
There was a problem hiding this comment.
I just un did the change, you will see the error I believe.
There was a problem hiding this comment.
======================================================================
ERROR: test_parse_target (test.shared_setup.TestSetup)
Test for parse target
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/test/shared_setup.py", line 193, in test_parse_target
new_target = setup.parse_target(package)
File "lib/vsc/install/shared_setup.py", line 1279, in parse_target
for name in new_target['cmdclass']:
RuntimeError: dictionary changed size during iterationThere was a problem hiding this comment.
I just pushed the fix again. Let me know how you'd like to proceed for the args/ kwargs string checking! The changes I had originally done adjust the regular expression to handle the different orders (and not fail tests).
There was a problem hiding this comment.
Ah, I think you should .copy new_target['cmdclass'] rather than new_target itself (or take a deepcopy of the entire new_target)...
lib/vsc/install/shared_setup.py
Outdated
| except AttributeError: | ||
| del new_target['cmdclass'][name] | ||
| del keepers['cmdclass'][name] | ||
| log.info("Not including new_target['cmdclass']['%s']" %name) |
There was a problem hiding this comment.
style nitpicking: missing space before name...
There was a problem hiding this comment.
where is the pep8 robot when you need him?!
There was a problem hiding this comment.
Yeah, maybe we should enable https://houndci.com/ here too, it has worked out quite well in the EasyBuild repos...
test/shared_setup.py
Outdated
| self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M)) | ||
| self.assertTrue(re.search(r"^kwargs:\s*\{.*'name':\s*'vsc-test'", txt, re.M)) | ||
| self.assertTrue(re.search(r"args:.*\(\)", txt, re.M)) | ||
| #self.assertTrue(re.search(r"kwargs:.*\{.*'name':.*'vsc-test'", txt, re.M)) |
There was a problem hiding this comment.
@vsoch No need to comment this out?
There was a problem hiding this comment.
I think we can test for a (general) pattern match to find the url key within the kwargs, but the specificity here (when you get different orderings) I can't get to work. I'll update the PR with something that does seem to work, and we can chat about if there is a better test.
There was a problem hiding this comment.
I don't think this specifies that stuff needs to be in a particular order in the original form?
Due to the .* in {.*'name' any other keys can be listed before 'name'?
test/shared_setup.py
Outdated
|
|
||
| self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M)) | ||
| self.assertTrue(re.search(r"^kwargs:\s*\{.*'name':\s*'vsc-test'", txt, re.M)) | ||
| self.assertTrue(re.search(r"args:.*\(\)", txt, re.M)) |
There was a problem hiding this comment.
@vsoch You can put back the ^ after fixing the print statements in fake_setup above?
These checks shouldn't need any tweaks imho...
There was a problem hiding this comment.
I think they would, they return different orderings of the keys?
There was a problem hiding this comment.
These tests were originally failing because the print statements fake_setup produced different output when using Python 3.x, but now they shouldn't anymore because you changed them...
The original checks should now (still) pass, even with ^args:\s* and ^kwargs:\s*?
There was a problem hiding this comment.
test/shared_setup.py
Outdated
|
|
||
| self.mock_stdout(True) | ||
| action_target({'name': 'vsc-test', 'version': '1.0.0'}, setupfn=fake_setup, urltemplate='http://example.com/%(name)s') | ||
| txt = self.get_stdout() | ||
| self.mock_stdout(False) | ||
| self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M)) | ||
| self.assertTrue(re.search(r"kwargs:.*\{.*'name':.*'vsc-test'", txt, re.M)) | ||
| #self.assertTrue(re.search(r"args:.*\(\)", txt, re.M)) |
There was a problem hiding this comment.
Same here, please restore to it's former glory now fake_setup is fixed?
|
@boegel here is the output we are testing! See how the first thing is "dependency links?" cat text-output.txt
INFO: run_tests from base dir /home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install (using executable /home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/setup.py)
INFO: initial packages list: ['vsc.install', 'vsc']
INFO: generated list: ['vsc.install', 'vsc']
INFO: generated packages list: ['vsc.install', 'vsc']
INFO: makesetupcfg set to True, (re)creating setup.cfg
INFO: found license /home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/LICENSE with md5sum 5f30f0716dfdd0d91eb439ebec522ec2
INFO: Found license name LGPLv2+ and classifier License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)
INFO: setting license LGPLv2+
INFO: found match url git@github.com:hpcugent/vsc-install in /home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/.git/config
INFO: found match name vsc-install in /home/vanessa/Documents/Dropbox/Code/Python/easybuild/vsc-install/.git/config
INFO: reg found: ('github.com', 'hpcugent/vsc-install')
INFO: Removing None download_url
INFO: get_name_url returns {'url': 'https://github.com/hpcugent/vsc-install', 'name': 'vsc-install'}
INFO: Name defined, not using auto determined name
INFO: using long_description vsc-install provides shared setuptools functions and classes for python libraries developed by UGent's HPC group
INFO: generated list: ['bin/python-noenv.sh', 'bin/python-stripped-env']
INFO: generated scripts list: ['bin/python-noenv.sh', 'bin/python-stripped-env']
INFO: adding prospector to tests_require
{'dependency_links': ['git+ssh://git@github.ugent.be/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+https://github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.ugent.be/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+https://github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33'], 'description': "vsc-install provides shared setuptools functions and classes for python libraries developed by UGent's HPC group", 'license': 'LGPLv2+', 'namespace_packages': ['vsc'], 'install_requires': [], 'name': 'vsc-test', 'scripts': ['bin/python-noenv.sh', 'bin/python-stripped-env'], 'setup_requires': ['setuptools', 'vsc-install >= 0.10.33'], 'download_url': '', 'long_description': 'Description\n===========\nvsc-install provides shared setuptools functions and classes for python libraries developed by UGent\'s HPC group\n\nCommon pitfalls\n=========\nbdist_rpm will fail if your install_requires = \'setuptools\' because it will fail to find a setuptools rpm.\n```\nexport VSC_RPM_PYTHON=1\n```\nwill make sure the `python-` prefix is added to the packages in install_requires for building RPM\'s so python-setuptools will be used.\n\nAdd tests\n=========\n\nTest are python modules in the `test` directory which have subclass of `TestCase`\nand at least one method that has a name starting with `test_`\n\nYou are advised to use\n```python\nfrom vsc.install.testing import TestCase\n```\n(instead of basic `TestCase` from `unittest`).\n\nAnd any `__main__` or `suite()` is not needed (anymore).\n\nInitialise the test directory with\n\n```bash\nmkdir -p test\necho \'\' > test/__init__.py\necho \'from vsc.install.commontest import CommonTest\' > test/00-import.py\n```\n\nWhen the tests are run, `test`, `lib` and `bin` (if relevant) are added to `sys.path`,\nso no need to do so in the tets modules.\n\nRun tests\n=========\n\n```bash\npython setup.py test\n```\n\nFilter tests with `-F` (test module names) and `-f` (test method names)\n\nSee also\n\n```bash\npython setup.py test --help\n```\n\nIn case following error occurs, it means there is a test module `XYZ` that cannot be imported.\n\n```txt\nFile "setup.py", line 499, in loadTestsFromModule\n testsuites = ScanningLoader.loadTestsFromModule(self, module)\nFile "build/bdist.linux-x86_64/egg/setuptools/command/test.py", line 37, in loadTestsFromModule\nFile "/usr/lib64/python2.7/unittest/loader.py", line 100, in loadTestsFromName\n parent, obj = obj, getattr(obj, part)\nAttributeError: \'module\' object has no attribute \'XYZ\'\n```\n\nYou can try get the actual import error for fixing the issue with\n```bash\npython -c \'import sys;sys.path.insert(0, "test");import XYZ;\'\n```\n\nFix failing tests\n=================\n\n* Missing / incorrect `LICENSE`\n\n * Copy the appropirate license file under `known_licenses` in the project directory and name the file `LICENSE`\n\n* Missing `README.md`\n\n * Create a `README.md` file with at least a `Description` section\n\n* Fix license headers as described in https://github.com/hpcugent/vsc-install/blob/master/lib/vsc/install/headers.py\n\n ```\n cd <project dir with .git folder>\n REPO_BASE_DIR=$PWD python -m vsc.install.headers path/to/file script_or_not\n ```\n\n Fix them all at once using find\n\n ```\n find ./{lib,test} -type f -name \'*.py\' | REPO_BASE_DIR=$PWD xargs -I \'{}\' python -m vsc.install.headers \'{}\'\n find ./bin -type f -name \'*.py\' | REPO_BASE_DIR=$PWD xargs -I \'{}\' python -m vsc.install.headers \'{}\' 1\n ```\n\n Do not forget to check the diff\n* Python scripts (i.e. with a python shebang and installed as scripts in setup) have to use `#!/usr/bin/env python` as shebang\n* Remove any `build_rpms_settings.sh` leftovers\n* The `TARGET` dict in `setup.py` should be minimal unless you really know what you are doing (i.e. if it is truly different from defaults)\n\n * Remove `name`, `scripts`, ...\n\n* `Exception: vsc namespace packages do not allow non-shared namespace`\n\n * Add to the `__init__.py`\n\n ```python\n """\n Allow other packages to extend this namespace, zip safe setuptools style\n """\n import pkg_resources\n pkg_resources.declare_namespace(__name__)\n ```\n\n\nbare-except\n-----------\n```python\ntry:\n # something\nexcept:\n```\nThis is bad, because this except will also catch sys.exit() or Keyboardinterupts, something you\ntypically do not want, if you catch these the program will be in a weird state and then continue on,\nwhilst the person who just pressed ctrl+c is wondering what is going on and why it is not stopping.\n\nso at the very least make this\nexcept Exception (which doesn\'t catch sys.exit and KeyboardInterupt)\nand it would be appreciated if you could actually figure out what exceptions to expect and only catch those\nand let your program crash if something you did not intend happens\nbecause it helps developers catch weird errors on their side better.\n\nif you do something like\n```python\ntry:\n open(int(somestring)).write(\'important data\')\nexcept Exception:\n pass # if somestring is not an integer, we didn\'t need to write anyway, but otherwise we do\n```\nbecause you know sometimes this string does not contain an integer, so the int() call can fail\nyou should really only catch ValueError, because this will also fail when your disk is full, or you don\'t have permissions\nor xxx other reasons, and the important data will not be written out and nobody will notice anything!\n\n\n\nif not \'a\' in somelist -> if \'a\' not in somelist\n-------------------------------------------------\n\nthis isn\'t that big of a deal, but if everyone is consistent it\'s less likely to introduce bugs when a not is added or removed where it didn\'t need to.\nAlso helps code review, not in reads better, like english.\n\n\narguments-differ\n-----------------\n\nthis will give you errors if you override a function of a superclass but don\'t use the same amount of arguments,\nusing less will surely give you errors, so the linter catches this for you now\n\nunused-argument\n-----------------\nif you have a function definition witch accepts an argument that is never used in the function body this will now give an error.\nclean up your function definition, or fix the error where you actually do need to take this argument into account\n\nunused-variable\n----------------\ndefining a variable and then not using it anymore smells bad, why did you do that?\nsometimes you do things like\n```python\nout, exit_code = run_command(something)\n```\nbut you are not interested in the out, only in the exit code,\nin this case, write\n```python\n_, exit_code = run_command(something)\n```\n\nusing _ as a variable name lets pylint and other readers know you do not intend to use that output in the first place.\n\n\nreimported\n-------------\nwhen you re import a name somewhere else,\nusually this is just an import to much, or 2 imports with the same name, pay attention.\n```python\nimport six\nfrom django import six\n```\n=>\n```python\nimport six\nfrom django import six as django_six\n```\n\nredefinition of unused name\n----------------------------\nthis usually also points to something you did not expect\n```python\nfrom vsc.accountpageclient import VscGroup\n<snip>\n\nclass VscGroup(object):\n pass\n```\n\n=> do you need the import? use import as\ndid you mean to use the same name? ...\n\nunpacking-in-except / redefine-in-handler\n-----------------------------------------\n\nMultiple exception have to be grouped in a tuple like\n\n```python\n ...\nexcept (ExceptionOne, ExceptionTwo) ...\n ...\n```\n\n(espcially when used like `except A, B:` which should be `except (A, B):`.\n\nturning off these errors\n-------------------------\n\n\nIf in any of these cases you think: yes, I really needed to do this,\nI\'m monkeypatching things, I\'m adding extra functionality that does indeed have an extra(default) paramenter, etc, etc\nyou can let pylint know to ignore this error in this one specific block of code\nby adding e.g. the comment `# pylint: disable=<name or numeric id of pylint code>`\n\n```python\nclass Something(object):\n def dosomething(self, some, thing):\n # do something\n\nclass MyFancyThing(SomeThing):\n # pylint: disable=arguments-differ\n def dosomething(self, some, thing, fancy=None):\n # do something fancy\n```\n\nFull list with all codes is available at http://pylint-messages.wikidot.com/all-codes\n', 'url': 'https://github.com/hpcugent/vsc-install', 'test_suite': 'test', 'version': '1.0.0', 'cmdclass': {'bdist_rpm': <class vsc.install.shared_setup.vsc_bdist_rpm at 0x7f368de44e20>, 'install_scripts': <class vsc.install.shared_setup.vsc_install_scripts at 0x7f368de44d50>, 'egg_info': <class vsc.install.shared_setup.vsc_egg_info at 0x7f368de44c80>, 'test': <class vsc.install.shared_setup.VscTestCommand at 0x7f368de44e88>, 'vsc_release': <class vsc.install.shared_setup.vsc_release at 0x7f368de44ef0>, 'sdist': <class vsc.install.shared_setup.vsc_sdist at 0x7f368de44bb0>}, 'command_packages': ['vsc.install.shared_setup', 'shared_setup_dist_only', 'setuptools.command', 'distutils.command'], 'packages': ['vsc.install', 'vsc'], 'classifiers': ['License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)'], 'tests_require': ['prospector >= 0.12.1', 'pylint-django == 0.9.1'], 'package_dir': {'': 'lib'}}
('args: ', ())
('kwargs:', {'dependency_links': ['git+ssh://git@github.ugent.be/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+https://github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.ugent.be/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+ssh://git@github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33', 'git+https://github.com/hpcugent/vsc-install.git#egg=vsc-install-0.10.33'], 'cmdclass': {'bdist_rpm': <class vsc.install.shared_setup.vsc_bdist_rpm at 0x7f368de44e20>, 'install_scripts': <class vsc.install.shared_setup.vsc_install_scripts at 0x7f368de44d50>, 'egg_info': <class vsc.install.shared_setup.vsc_egg_info at 0x7f368de44c80>, 'test': <class vsc.install.shared_setup.VscTestCommand at 0x7f368de44e88>, 'vsc_release': <class vsc.install.shared_setup.vsc_release at 0x7f368de44ef0>, 'sdist': <class vsc.install.shared_setup.vsc_sdist at 0x7f368de44bb0>}, 'name': 'vsc-test', 'license': 'LGPLv2+', 'install_requires': [], 'setup_requires': ['setuptools', 'vsc-install >= 0.10.33'], 'download_url': '', 'classifiers': ['License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)'], 'namespace_packages': ['vsc'], 'test_suite': 'test', 'url': 'https://github.com/hpcugent/vsc-install', 'version': '1.0.0', 'scripts': ['bin/python-noenv.sh', 'bin/python-stripped-env'], 'package_dir': {'': 'lib'}, 'command_packages': ['vsc.install.shared_setup', 'shared_setup_dist_only', 'setuptools.command', 'distutils.command'], 'packages': ['vsc.install', 'vsc'], 'long_description': 'Description\n===========\nvsc-install provides shared setuptools functions and classes for python libraries developed by UGent\'s HPC group\n\nCommon pitfalls\n=========\nbdist_rpm will fail if your install_requires = \'setuptools\' because it will fail to find a setuptools rpm.\n```\nexport VSC_RPM_PYTHON=1\n```\nwill make sure the `python-` prefix is added to the packages in install_requires for building RPM\'s so python-setuptools will be used.\n\nAdd tests\n=========\n\nTest are python modules in the `test` directory which have subclass of `TestCase`\nand at least one method that has a name starting with `test_`\n\nYou are advised to use\n```python\nfrom vsc.install.testing import TestCase\n```\n(instead of basic `TestCase` from `unittest`).\n\nAnd any `__main__` or `suite()` is not needed (anymore).\n\nInitialise the test directory with\n\n```bash\nmkdir -p test\necho \'\' > test/__init__.py\necho \'from vsc.install.commontest import CommonTest\' > test/00-import.py\n```\n\nWhen the tests are run, `test`, `lib` and `bin` (if relevant) are added to `sys.path`,\nso no need to do so in the tets modules.\n\nRun tests\n=========\n\n```bash\npython setup.py test\n```\n\nFilter tests with `-F` (test module names) and `-f` (test method names)\n\nSee also\n\n```bash\npython setup.py test --help\n```\n\nIn case following error occurs, it means there is a test module `XYZ` that cannot be imported.\n\n```txt\nFile "setup.py", line 499, in loadTestsFromModule\n testsuites = ScanningLoader.loadTestsFromModule(self, module)\nFile "build/bdist.linux-x86_64/egg/setuptools/command/test.py", line 37, in loadTestsFromModule\nFile "/usr/lib64/python2.7/unittest/loader.py", line 100, in loadTestsFromName\n parent, obj = obj, getattr(obj, part)\nAttributeError: \'module\' object has no attribute \'XYZ\'\n```\n\nYou can try get the actual import error for fixing the issue with\n```bash\npython -c \'import sys;sys.path.insert(0, "test");import XYZ;\'\n```\n\nFix failing tests\n=================\n\n* Missing / incorrect `LICENSE`\n\n * Copy the appropirate license file under `known_licenses` in the project directory and name the file `LICENSE`\n\n* Missing `README.md`\n\n * Create a `README.md` file with at least a `Description` section\n\n* Fix license headers as described in https://github.com/hpcugent/vsc-install/blob/master/lib/vsc/install/headers.py\n\n ```\n cd <project dir with .git folder>\n REPO_BASE_DIR=$PWD python -m vsc.install.headers path/to/file script_or_not\n ```\n\n Fix them all at once using find\n\n ```\n find ./{lib,test} -type f -name \'*.py\' | REPO_BASE_DIR=$PWD xargs -I \'{}\' python -m vsc.install.headers \'{}\'\n find ./bin -type f -name \'*.py\' | REPO_BASE_DIR=$PWD xargs -I \'{}\' python -m vsc.install.headers \'{}\' 1\n ```\n\n Do not forget to check the diff\n* Python scripts (i.e. with a python shebang and installed as scripts in setup) have to use `#!/usr/bin/env python` as shebang\n* Remove any `build_rpms_settings.sh` leftovers\n* The `TARGET` dict in `setup.py` should be minimal unless you really know what you are doing (i.e. if it is truly different from defaults)\n\n * Remove `name`, `scripts`, ...\n\n* `Exception: vsc namespace packages do not allow non-shared namespace`\n\n * Add to the `__init__.py`\n\n ```python\n """\n Allow other packages to extend this namespace, zip safe setuptools style\n """\n import pkg_resources\n pkg_resources.declare_namespace(__name__)\n ```\n\n\nbare-except\n-----------\n```python\ntry:\n # something\nexcept:\n```\nThis is bad, because this except will also catch sys.exit() or Keyboardinterupts, something you\ntypically do not want, if you catch these the program will be in a weird state and then continue on,\nwhilst the person who just pressed ctrl+c is wondering what is going on and why it is not stopping.\n\nso at the very least make this\nexcept Exception (which doesn\'t catch sys.exit and KeyboardInterupt)\nand it would be appreciated if you could actually figure out what exceptions to expect and only catch those\nand let your program crash if something you did not intend happens\nbecause it helps developers catch weird errors on their side better.\n\nif you do something like\n```python\ntry:\n open(int(somestring)).write(\'important data\')\nexcept Exception:\n pass # if somestring is not an integer, we didn\'t need to write anyway, but otherwise we do\n```\nbecause you know sometimes this string does not contain an integer, so the int() call can fail\nyou should really only catch ValueError, because this will also fail when your disk is full, or you don\'t have permissions\nor xxx other reasons, and the important data will not be written out and nobody will notice anything!\n\n\n\nif not \'a\' in somelist -> if \'a\' not in somelist\n-------------------------------------------------\n\nthis isn\'t that big of a deal, but if everyone is consistent it\'s less likely to introduce bugs when a not is added or removed where it didn\'t need to.\nAlso helps code review, not in reads better, like english.\n\n\narguments-differ\n-----------------\n\nthis will give you errors if you override a function of a superclass but don\'t use the same amount of arguments,\nusing less will surely give you errors, so the linter catches this for you now\n\nunused-argument\n-----------------\nif you have a function definition witch accepts an argument that is never used in the function body this will now give an error.\nclean up your function definition, or fix the error where you actually do need to take this argument into account\n\nunused-variable\n----------------\ndefining a variable and then not using it anymore smells bad, why did you do that?\nsometimes you do things like\n```python\nout, exit_code = run_command(something)\n```\nbut you are not interested in the out, only in the exit code,\nin this case, write\n```python\n_, exit_code = run_command(something)\n```\n\nusing _ as a variable name lets pylint and other readers know you do not intend to use that output in the first place.\n\n\nreimported\n-------------\nwhen you re import a name somewhere else,\nusually this is just an import to much, or 2 imports with the same name, pay attention.\n```python\nimport six\nfrom django import six\n```\n=>\n```python\nimport six\nfrom django import six as django_six\n```\n\nredefinition of unused name\n----------------------------\nthis usually also points to something you did not expect\n```python\nfrom vsc.accountpageclient import VscGroup\n<snip>\n\nclass VscGroup(object):\n pass\n```\n\n=> do you need the import? use import as\ndid you mean to use the same name? ...\n\nunpacking-in-except / redefine-in-handler\n-----------------------------------------\n\nMultiple exception have to be grouped in a tuple like\n\n```python\n ...\nexcept (ExceptionOne, ExceptionTwo) ...\n ...\n```\n\n(espcially when used like `except A, B:` which should be `except (A, B):`.\n\nturning off these errors\n-------------------------\n\n\nIf in any of these cases you think: yes, I really needed to do this,\nI\'m monkeypatching things, I\'m adding extra functionality that does indeed have an extra(default) paramenter, etc, etc\nyou can let pylint know to ignore this error in this one specific block of code\nby adding e.g. the comment `# pylint: disable=<name or numeric id of pylint code>`\n\n```python\nclass Something(object):\n def dosomething(self, some, thing):\n # do something\n\nclass MyFancyThing(SomeThing):\n # pylint: disable=arguments-differ\n def dosomething(self, some, thing, fancy=None):\n # do something fancy\n```\n\nFull list with all codes is available at http://pylint-messages.wikidot.com/all-codes\n', 'tests_require': ['prospector >= 0.12.1', 'pylint-django == 0.9.1'], 'description': "vsc-install provides shared setuptools functions and classes for python libraries developed by UGent's HPC group"})These tests would be meaningful to check that the keys exist somewhere in the dict. But it feels a bit like "overfitting in the testing world" to expect an exact location of the key. |
|
@stdweird yours to give it another review? |
lib/vsc/install/commontest.py
Outdated
| # Prospector doesn;t have support for 3.5 / 3.6 | ||
| # https://github.com/PyCQA/prospector/issues/233 | ||
| if sys.version_info >= (3, 5): | ||
| HAS_PROTECTOR = False |
There was a problem hiding this comment.
can you fix the typo everywhere? (@JensTimmerman unless this is intentional)
There was a problem hiding this comment.
The doesn;t is my mistake, and I'd be happy to fix that!
| @@ -95,7 +102,6 @@ class CommonTest(TestCase): | |||
| 'undefined', | |||
| 'no-value-for-parameter', | |||
| 'dangerous-default-value', | |||
| 'redefined-builtin', | |||
There was a problem hiding this comment.
why is this removed? isn't adding the blacklist entry sufficient?
There was a problem hiding this comment.
I think it is being removed in favor of the blacklist entry, which is more specific for the intended bullitin:
"Redefining built-in 'basestring'"
There was a problem hiding this comment.
i think this disables all redefined builtin errors. you should keep this one
lib/vsc/install/shared_setup.py
Outdated
| import builtins as __builtin__ | ||
|
|
||
| try: | ||
| basestring # Python 2 |
There was a problem hiding this comment.
what is this trying to achieve?
There was a problem hiding this comment.
basestring (strangely enough) isn't defined for Python 3, so if we try to use it in the code, we get a NameError. Instead of the error getting triggered here (and the tests fail for python 3 because the type isn't valid)
if isinstance(name, basestring):
...
we add this statement to the top of the file:
try:
basestring # Python 2
except NameError:
basestring = (bytes, str) # Python 3And given we are in Python 2, basestring exists and the statement does nothing (but is valid) and continues on. If we are in Python 3, just typing "basestring" will trigger the error! This is where we want to catch it, and redefine it as the Python 3 matching type.
Depending on the implementation, the other alternative would be to have the check be for a type that is supported across the versions. For Python 3, we will undoubtedly have to deal with "sometimes bytes, sometimes str" and I believe this is why @boegel chose to define basestring in this way.
There was a problem hiding this comment.
Maybe it's better/cleaner to make it more explicit, like this (collapsed with the above):
if sys.version_info < (3, 0):
import __builtin__
else:
basestring = (bytes, str) # make sure 'basestring' (a builtin in Python 2.x) is defined when using Python 3
import builtins as __builtin__ # make builtins accessible via same way as in Python 3
| @@ -361,13 +373,15 @@ def get_name_url(self, filename=None, version=None, license_name=None): | |||
| if len(res) != 3: | |||
| raise Exception("Cannot determine name, url and download url from filename %s: got %s" % (filename, res)) | |||
| else: | |||
| keepers = {} | |||
There was a problem hiding this comment.
out of curiosity, why is this modified?
There was a problem hiding this comment.
The previous strategy was starting with a dictionary, res, and removing items from it as it was being iterated through. This triggers errors with testing, and generally isn't a good idea! To fix, I decided to add keepers, and instead of removing from res, I am adding entries from res to keepers that are intended to return. We then return "keepers" instead of res.
There was a problem hiding this comment.
To clarify: this is required because in Python 3 .items() returns an iterator, so modifying the dict we're iterating over means trouble in that case. In Python 2, .items() return a list (a copy of the contents of the dict), so modifying the original dict while iterating over .items() is fine in that case.
| @@ -1254,11 +1275,15 @@ def parse_target(self, target, urltemplate=None): | |||
|
|
|||
| # update the cmdclass with ones from vsc_setup_klass | |||
| # cannot do this in one go, when SHARED_TARGET is defined, vsc_setup doesn't exist yet | |||
| for name, klass in new_target['cmdclass'].items(): | |||
| keepers = new_target['cmdclass'].copy() | |||
There was a problem hiding this comment.
out of curiosity, why is this modified
There was a problem hiding this comment.
The same issue is here as for the previous keepers - you can't iterate through a dictionary and change it at the same time.
There was a problem hiding this comment.
The copy() creates a new copy in memory. If we didn't do that, we would just be pointing to the same dictionary.
There was a problem hiding this comment.
Same here: .items() is a list (copy) in Python 2, while in Python 3 it's an iterator
… redefined-bulletin to blacklist
lib/vsc/install/commontest.py
Outdated
| # Prospector doesn't have support for 3.5 / 3.6 | ||
| # https://github.com/PyCQA/prospector/issues/233 | ||
| if sys.version_info >= (3, 5): | ||
| HAS_PROTECTOR = False |
There was a problem hiding this comment.
i meant this (old, not your) typo: HAS_PROTECTOR -> HAS_PROSPECTOR
There was a problem hiding this comment.
haha, I didn't even notice that bit! In that variable names should be meaningful, and in that this is referring to prospector, I too think this should be corrected. I don't see the variable anywhere else in the vsc-install and did a grep for vsc-base as well, so as long as it's not imported in any of the lower lower vsc tools modules, it should be ok to change.
lib/vsc/install/commontest.py
Outdated
| 'Locally disabling', # shows up when you locally disable a warning, this is the point | ||
| 'Useless suppression', # shows up when you locally disable/suppress a warning, this is the point | ||
| 'redefined-builtin', |
There was a problem hiding this comment.
sorry to be clear - it originally was on the blacklist, and you want it removed from the blacklist and added instead to the white list?
lib/vsc/install/commontest.py
Outdated
| @@ -87,15 +93,16 @@ class CommonTest(TestCase): | |||
| # Whitelist: if match, fail test | |||
| PROSPECTOR_BLACKLIST = [ | |||
| # 'wrong-import-position', # not sure about this, these usually have a good reason | |||
| "Redefining built-in 'basestring'", # basestring is defined when running on top of Python 3.x | |||
There was a problem hiding this comment.
actually, you should remove this one too. just ack the pylint failure on the 2 lines in vsc-install where you do the redefinition.
for all other repos that use this, it's better to introduce a is_string function in vsc.utils.missing, and in that module, also ack the redefinition (or better yet, make a function that doesn't require a redefinition). i'm going to assume that most use cases of basestring are from isinstance(somestring, basestring), so that test should be modified everywhere (it's more work, but much cleaner)
lib/vsc/install/commontest.py
Outdated
| @@ -124,6 +129,7 @@ class CommonTest(TestCase): | |||
| 'old-ne-operator', # don't use <> as not equal operator, use != | |||
| 'backtick', # don't use `variable` to turn a variable in a string, use the str() function | |||
| 'old-raise-syntax', # sed when the alternate raise syntax raise foo, bar is used instead of raise foo(bar) . | |||
| 'redefined-builtin', | |||
| # once we get ready to really move to python3 | |||
| # 'print-statement', # use print() and from future import __print__ instead of print | |||
| # 'metaclass-assignment', # __metaclass__ doesn't exist anymore in python3 | |||
There was a problem hiding this comment.
you can uncomment above 2 error classes now :)
|
Thanks a lot for this work already! |
|
hey @stdweird and @JensTimmerman are there any more issues for me to address? |
lib/vsc/install/testing.py
Outdated
| @@ -62,11 +67,11 @@ def assertEqual(self, a, b, msg=None): | |||
| else: | |||
| msg = "%s: %s" % (msg, e) | |||
|
|
|||
| if isinstance(a, basestring): | |||
| if isinstance(a, str): | |||
There was a problem hiding this comment.
why change it like this?
make a testfunction
def is_string(x):
try:
return isinstance(x, basestring)
except NameError:
return isinstance(x, str)(maybe last one (ie py3 case) should be isinstance(x, (str, bytes)), but i'm not sure
also, reintroduce this function in vsc-base (utils.missing) for all other repos. (we do not want that vsc-install becomes a runtime dependency of anything, should only be needed for packaging and testing)
There was a problem hiding this comment.
The goal was to remove basestring entirely - with that function you will get broken tests again.
There was a problem hiding this comment.
Any reason to not use six.string_types ? https://pythonhosted.org/six/#six.string_types
There was a problem hiding this comment.
what tests are broken with that function?
wrt six, i thought you wanted to avoid six?
There was a problem hiding this comment.
If you're reimplementing six you could reimplement int in the same way, and have vsc.utils.missing contain a string_types that's defined based on the version of python we're running, or copy all of six into vsc.utils.six?
But if this is the only case where six would come in handy, this is ok I suppose.
|
I take it back, there is no issue with |
|
Something to get excited about for Python 3.7! :) https://realpython.com/python-data-classes/#immutable-data-classes |
| @@ -1047,7 +1055,7 @@ def finalize_options(self): | |||
|
|
|||
| def _print(self, cmd): | |||
| """Print is evil, cmd is list""" | |||
| print ' '.join(cmd) | |||
| print(' '.join(cmd)) | |||
There was a problem hiding this comment.
add the future print_function import to this module, since we're doing print here.
There was a problem hiding this comment.
okay, added to import! Please take a look and let me know if this is what you had in mind. And apologies for the delay - I was flying through the sky!!
…is this what you had in mind?
|
woohoo!! :D |
This PR includes minimal changes to update to Python three. In summary, this largely includes:
There were also a couple of instances of popping items from dictionaries while they were being iterated over, and I changed this to create a copy and then simply not add entries that would be removed. I also changed some of the file reading patterns to go from:
to more of:
I'm not sure how everything is working together, but to test I did:
and there were many issues with pylint not closing a file, but I believe that's out of our control here? The tests, as run above, pass with these tiny changes. I'm going to move over to the other repo vsc-base and see what kind of issues I run into now that I have vsc installed! I'll update here if necessary.