-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Store config in setup.cfg and update setuptools configuration #57
Conversation
8351971
to
850ae9c
Compare
There was a discussion about doing this in #56 and pypa/packaging.python.org#378 and the conclusion was that it's a bit too early to move to Personally, I like the feature, but I can see that we want to be cautious of making it the standard when there's likely to be plenty of people on older versions of tools. BTW, it would be easier to manage this PR if you focused it just on the |
a3f30a9
to
1de805d
Compare
@pfmoore Thanks for your comment. travis modifications not introduce changes on tests just import python version specificaly. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, as noted in the issues I referenced, I don't think this will be accepted in the short term. But if you want to prepare something that could be considered in the future, then there are a number of issues that I'd like to see addressed.
.travis.yml
Outdated
|
||
script: tox | ||
script: | ||
- tox -e $(echo py$TRAVIS_PYTHON_VERSION | tr -d .) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file are not related to switching to setup.cfg
, so they should be split out into a separate PR (with an explanation of why the changes are needed - are the tests failing as things stand?).
sample/__init__.py
Outdated
|
||
|
||
def _extract_version(package_name): | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is code that we're recommending everyone include in their projects - I don't think we want an _extract_version
like this, or pkg_resources
and setuptools
dependencies, in such a recommendation.
The existing code doesn't offer a "get the package version" function, or try to address how the application should expose its version at runtime. I'd rather that remain the case, but even if we were to decide that we do that, it should be a separate PR (again!)
sample/__init__.py
Outdated
return _conf['metadata']['version'] | ||
|
||
|
||
__version__ = _extract_version('pipenv') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the reference to pipenv
here?
sample/__init__.py
Outdated
|
||
def main(): | ||
"""Entry point for the application script""" | ||
print("Sample version {}".format(__version__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a stub for people to write their application code - why print the version here?
sample/__init__.py
Outdated
|
||
|
||
if __name__ == "__main__": | ||
print(__version__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__init__.py
isn't typically executable. What's the point of this change?
MANIFEST.in
Outdated
@@ -3,3 +3,6 @@ include LICENSE.txt | |||
|
|||
# Include the data files | |||
recursive-include data * | |||
|
|||
# Include package data | |||
include sample/package_data.dat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just because of the "if you are using Python 2.6, it should be in MANIFEST.in
comment below? If so, then (a) it's not specific to this PR, it's a change that applies to the existing code as well, but (b) we don't support Python 2.6 according to setup.py
/setup.cfg
, so we don't need it anyway.
09181c9
to
41825c6
Compare
@pfmoore changes applied! |
41825c6
to
6cf9e15
Compare
https://packaging.python.org/tutorials/distributing-packages/#setup-cfg I think this documentation must be updated, it's reference setup.cfg on sampleproject but sampleproject don't use this practice at this moment |
The docs refer to Just because we don't give an example of (or recommend) the relatively new setuptools extension to allow most of |
@pfmoore ok thanks for response :) |
[Fix] Add zip_safe option
Overview
Move setup configuration to setup.cfg insteadof setup.py.
Features