-
Notifications
You must be signed in to change notification settings - Fork 98
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
Moved the metadata into setup.cfg #127
Conversation
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.
Hi there,
I was just wandering around and found a few thing I thought you may want to reconsider. I am not a maintainer of this or anything, just a concerned user whose packages are dependent on this gem.
Cheers.
setup.py
Outdated
for line in read("appdirs.py").splitlines(): | ||
if line.startswith("__version__"): | ||
version = ast.literal_eval(line.split("=", 1)[1].strip()) | ||
break |
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.
Please notice the inconsistency in indentation here. I also believe that double blank lines was to separate the lengthy setup
call from the preparation part (sort-of), so in this case it might not be necessary.
def read(fname): | ||
inf = open(os.path.join(os.path.dirname(__file__), fname)) | ||
inf = open(os.path.join(current_dir, fname), "rt", encoding="utf-8") |
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.
rt
are default for open
, so IMHO there won't be any gain being that explicit here. Additionally, if this gets merge,, the only call for read
is to get the version, thus the code below could be refactored to
with open(os.path.join(op.path.dirname(__file__), "appdirs.py")) as source:
version = next(ast.literal_eval(line.split("=", 1)[1].strip())
for line in source if line.startswith("__version__"))
|
||
setup_requires = setuptools>=30.3.0; wheel |
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.
I just want to remind you about b68a72e that setuptools
is actually not required. Though, if distutils
reads the setup_requires
option or not, I am not exactly sure. Forgive me if it does not.
Added pyproject.toml to allow building with `build`.
I'd recommend to close this in favor of #136. |
No description provided.