Skip to content
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

use latest pip on travis and enable caching #100

Merged
merged 21 commits into from
Dec 11, 2017

Conversation

thijstriemstra
Copy link
Collaborator

@thijstriemstra thijstriemstra commented Nov 14, 2017

  • also builds docs and does some pep8 checks on travis
  • moved version number to central place in flickrapi/__version__.py
  • lots of pep8 fixes (whitespace, missing lines etc)
  • bump version to 2.3.2

@thijstriemstra
Copy link
Collaborator Author

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

Coverage.py warning: No data was collected. (no-data-collected)

@thijstriemstra
Copy link
Collaborator Author

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

@sybrenstuvel
Copy link
Owner

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

@thijstriemstra
Copy link
Collaborator Author

sounds good, thx.

@coveralls
Copy link

Coverage Status

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

Repository owner deleted a comment from coveralls Nov 19, 2017
Repository owner deleted a comment from coveralls Nov 19, 2017
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@sybrenstuvel
Copy link
Owner

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
Copy link
Collaborator Author

thijstriemstra commented Nov 20, 2017

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

@sybrenstuvel
Copy link
Owner

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
Copy link

Coverage Status

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

@thijstriemstra
Copy link
Collaborator Author

@sybrenstuvel pinned the versions in setup.py

@sybrenstuvel
Copy link
Owner

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
Copy link
Owner

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

@thijstriemstra
Copy link
Collaborator Author

@sybrenstuvel gotcha, see update.

@coveralls
Copy link

Coverage Status

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

@sybrenstuvel
Copy link
Owner

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@thijstriemstra
Copy link
Collaborator Author

@sybrenstuvel restored the file and addressed your comments.

Copy link
Owner

@sybrenstuvel sybrenstuvel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good changes, thanks!

@sybrenstuvel sybrenstuvel merged commit e89c0e9 into sybrenstuvel:master Dec 11, 2017
@thijstriemstra thijstriemstra deleted the patch-1 branch January 9, 2018 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants