Reactivate Travis-CI #86
Conversation
| @@ -1,67 +1,71 @@ | |||
| git blanguage: python | |||
There was a problem hiding this comment.
Probably a typo that prevented Travis-CI from running all this time.
| @@ -1,67 +1,71 @@ | |||
| git blanguage: python | |||
| sudo: required | |||
| # if the https://travis-ci.org/ODM2/ODM2PythonAPI/requests ever says: missing config | |||
There was a problem hiding this comment.
We don't really need sudo and without it Travis-CI can run in a faster using the container infrastructure.
PS: I removed all the unnecessary comments.
| sudo: required | ||
| # if the https://travis-ci.org/ODM2/ODM2PythonAPI/requests ever says: missing config | ||
| # validate at: http://lint.travis-ci.org/ | ||
| python: |
There was a problem hiding this comment.
Changed this to a test matrix definition with only Python 2.7 and 3.6.
Python 3.6 is unsupported now but I aim to change that soon.
| # - "3.5-dev" # 3.5 development branch | ||
| # - "nightly" # currently points to 3.6-dev | ||
| # command to install dependencies | ||
| cache: |
There was a problem hiding this comment.
To avoid a big PR I will add a caching mechanism in another PR as soon as this is merged.
| - mysql-client | ||
| # FIXME: I don't see the need for these anywhere. |
There was a problem hiding this comment.
I don't really think we need any of these packages from the package manager.
Can someone with a more intimate knowledge of odm2api comment on this?
There was a problem hiding this comment.
Which packages? If it's libproj-dev, libgeos-dev and libspatialiate-dev, I agree, they're not needed. I can't comment on cmake.
| script: | ||
| - if [[ $TEST_TARGET == 'default' ]]; then | ||
| cp -r tests /tmp && cd /tmp ; | ||
| py.test -vv tests ; |
There was a problem hiding this comment.
This ensure that we are running the tests from the installed version and not the source code tree.
| - py.test | ||
|
|
||
| - if [[ $TEST_TARGET == 'coding_standards' ]]; then | ||
| find . -type f -name "*.py" ! -name 'conf.py' | xargs flake8 --max-line-length=100 ; |
There was a problem hiding this comment.
The coding standards tests, together with the Python 3 tests, are in the allow_failures section at the moment.
I plan to send small PRs to fix that and make these tests first class citizens here.
requirements-dev.txt
Outdated
| pymssql | ||
| psycopg2 | ||
| flake8 | ||
| # FIXME: I am not sure these are needed. |
There was a problem hiding this comment.
This needs a careful review.
I see packages dependencies all over the place and they don't really seems to be used.
I do know that, like for the wofpy case, people expected these to be installed even when the module in question is not using them directly.
If that is the case we should create an integration test env to add and test those and the expected functionality.
There was a problem hiding this comment.
It looks like you have left in all of the dependencies that I usually install when running the API, so this looks good to me.
There was a problem hiding this comment.
👍
Except, you have pymssql and psycopg2 listed twice
| @@ -1,15 +0,0 @@ | |||
| -r requirements.txt | |||
There was a problem hiding this comment.
Renamed to a more common name and removed the pip-ism syntax to make it package manager agnostic.
| # 'console_scripts': [ | ||
| # 'sample=sample:main', | ||
| # ], | ||
| # }, |
There was a problem hiding this comment.
Mostly cleanups. The comments are not really helpful and get in the way of reading the code here.
|
@sreeder and @lsetiawan, can you review this PR from @ocefpaf? I suspect some areas may be outside your individual expertise, but we can work together on this with @ocefpaf's guidance. I won't be able to provide input until tomorrow (Thursday), and then @ocefpaf will be traveling starting on Friday. Thanks! |
setup.py
Outdated
| version=versioneer.get_version(), | ||
|
|
||
| description='A Python-based application programmers interface for the Observations Data converter 2 (ODM2) ', | ||
| description='Python interface for the Observations Data converter 2 (ODM2)', |
There was a problem hiding this comment.
Isn't ODM2 stands for Observations Data Model 2?
There was a problem hiding this comment.
I only reduced the description that was already there, but I guess that we should also fix it to Model, right @emiliom?
setup.py
Outdated
| 'Programming Language :: Python :: 2.7', | ||
| 'Topic :: Software Development :: Libraries :: Python Modules', | ||
| 'Topic :: Scientific/Engineering' | ||
| ], | ||
|
|
||
| # What does your project relate to? | ||
| keywords='Observations Data converter ODM2', |
There was a problem hiding this comment.
Again, Observations Data Model 2 instead?
There was a problem hiding this comment.
I think that @ocefpaf changes looks good and heading in the right direction. I don't know enough about the package dependencies for odm2api to comment what are needed. Otherwise, I approve. Though some questions about Observations Data Model 2 instead of Observations Data Converter 2?
|
Everything looks good to me, except for the duplicated listing of |
Probably leftovers from a bad rebase. Latest commit should fix it. |
This is a substitute for #74
I made several small commits to make it easier to review.
It should be easier for the reviewer to navigate the commits in order.
Note that the tests are now running, but not passing.
Most of them no longer make sense b/c they are outdated and disconnected from the latest code.
We should not only write new meaningful tests, but also write them together with the new code.