sybrenstuvel / flickrapi

Python Flickr API implementation
https://stuvel.eu/flickrapi
Other
155 stars 33 forks source link

use latest pip on travis and enable caching #100

Closed thijstriemstra closed 6 years ago

thijstriemstra commented 6 years ago
thijstriemstra commented 6 years ago

This PR seems to reveal some test failures and broken coverage report:

Coverage.py warning: No data was collected. (no-data-collected)
thijstriemstra commented 6 years ago

ping @sybrenstuvel (you should enable github feature 'request review' for prs).

sybrenstuvel commented 6 years ago

I could make you collaborator, so that you can do that yourself. Would you like that?

thijstriemstra commented 6 years ago

sounds good, thx.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling dcc0fb61911cd38737a54ee9ffd9fb40c8bf60d4 on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling 32b5668c566086237faf5ddf7e29c2ee7f2abc52 on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling 1c08119c5e8c81f4448577fbbfa93fc048d2399b on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling 9d6bbb683442d9c2f56ffd40cb887aea4e29d0f5 on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

sybrenstuvel commented 6 years ago

Nice work!

The requirements.txt file contains the exact versions of all the dependencies I use for testing, and provides a known-good set of version numbers. This also makes it possible to have multiple machines (owned by the same or different developers) all with a consistent set of dependencies. As such, it can't be replaced by just the version numbers in setup.py (which indicate minimum versions and/or upper limits to the versions for compatibility reasons, but not specific version numbers).

I do agree that those versions should be updated at some point.

thijstriemstra commented 6 years ago

@sybrenstuvel I will define the latest and greatest in setup.py then?

sybrenstuvel commented 6 years ago

No, define the minimum required versions in the setup.py. Don't increase them unless there is a very good reason to. Having those versions as low as possible makes it much easier for packaging with Linux distributions, 3rd party applications, etc. as they don't have to mess with their other packages when pulling in an update to the FlickrAPI library.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling 43c2cee8c1ab6413629a03097060bebbf3c321b6 on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

thijstriemstra commented 6 years ago

@sybrenstuvel pinned the versions in setup.py

sybrenstuvel commented 6 years ago

I may not have been clear in my explanation. The versions should be mimimal versions, i.e. pkgname>=2.4, so that newer versions that have already been installed are also valid. requirements.txt should contain specific versions, i.e. pkgname==2.4.

sybrenstuvel commented 6 years ago

(version numbers there are for illustration only, they could very well be different)

thijstriemstra commented 6 years ago

@sybrenstuvel gotcha, see update.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling 29d84f5ac8a0939ddc07151d4a451d9311bc5500 on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

sybrenstuvel commented 6 years ago

Better, but why still delete requirements.txt?

In travis.yml you put pip install --ignore-installed --upgrade setuptools pip tox-travis coveralls. What's the effect of having both --ignore-installed and --upgrade in one invocation? Don't they clash?

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling c66396e16134f192f3dd99eb91f1057115245aad on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 71.156% when pulling c66396e16134f192f3dd99eb91f1057115245aad on thijstriemstra:patch-1 into e3e5072b097110701524805371c5000a1b8c0091 on sybrenstuvel:master.

thijstriemstra commented 6 years ago

@sybrenstuvel restored the file and addressed your comments.