Closed vheon closed 9 years ago
Took a look. You're using magic that I have no authority on, so just idle comments :) Otherwise - the tests pass and it all looks sane enough, so LGTM
Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.
travis/install.sh, line 28 [r1] (raw file): Is it intentional that the py26 and py27 cases are identical? When I looked at ycmd, OS X did not support the "python" language type and i had to rely on OS X's built-in multi-python-versioning stuff. That seemed to work.
What ensures that you're actually running python 2.6 for this branch for example? I actually added something to the script to fail the build if running python ran the wrong version.
# It is quite easy to get the above series of steps wrong. Verify that the
# version of python actually in the path and used is the version that was
# requested, and fail the build if we broke the travis setup
python_version=$(python -c 'import sys; print "{0}.{1}".format( sys.version_info[0], sys.version_info[1] )')
echo "Checking python version (actual ${python_version} vs expected ${YCMD_PYTHON_VERSION})"
test ${python_version} == ${YCMD_PYTHON_VERSION}
Sorry it might be that tox is doing it, (probably) or this is obvious to you - i've not studied tox, and i've never used pyenv
so I'm probably pretty useless as a reviewer for this :)
Comments from the review on Reviewable.io
Review status: all files reviewed at latest revision, 1 unresolved discussion.
travis/install.sh, line 28 [r1] (raw file):
Yes, as you know OS X came with built-in python 2.6 and python 2.7 so there is no need to install them, but you cannot tell Travis that you want to build and test python code because it will call scripts to set up the environment which are not ready on OS X. So what I did in the matrix is to specify language: general
for the OS X cases and for python 2.6 and 2.7 all I do is install pip. What garanties that the tests will run under the right python is tox
: it will search for the right version of python and complain if it can't find it.
Comments from the review on Reviewable.io
Review status: all files reviewed at latest revision, 1 unresolved discussion.
travis/install.sh, line 28 [r1] (raw file): That's pretty handy! I wish I knew about this when I did it for ycmd. It was like a world of pain... :D
Comments from the review on Reviewable.io
Review status: all files reviewed at latest revision, 1 unresolved discussion.
travis/install.sh, line 28 [r1] (raw file): Well for me i was easier because this project is all python while on ycmd we have a more complex setup and testing to do :+1:
Comments from the review on Reviewable.io
LGTM
Review status: all files reviewed at latest revision, 2 unresolved discussions.
travis/install.sh, line 22 [r1] (raw file): I'd recommend that after you clone the repo you also check out a specific commit hash. This way your travis runs are isolated from upstream changes; you can update at your leisure (after testing).
Comments from the review on Reviewable.io
I preferred to enable tests on OS X before making a PR for HMAC. If you guys want to take a look :)
@Valloric @puremourning @micbou @oblitum