-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix compiler warnings #54
Conversation
- Wrapped Python version specific variables in `ifdef`'s - Changed type in PyErr_Format to please Python 2.7 - Added Python requested metadata to setup.py
setup.py
Outdated
setup( | ||
name="ciso8601", | ||
version="2.0.1", | ||
description='Fast ISO8601 date time parser for Python written in C', | ||
long_description=long_description, | ||
author="Thomas Steinacher, Michael Overmeyer", | ||
author_email="ciso8601@movermeyer.com", |
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.
We'd probably want to either set up a group on our company domain (ciso8601@close.io which can go to your email as well), or use a neutral Google group. Or we just skip this field for now.
(I was also thinking about multiple individual email addresses, but see https://bugs.python.org/msg93284)
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.
Actually, I think I'll back out the author
and author_email
changes to this file.
I don't expect anyone to actually use the email, and the author
field is likely to get out of date.
If people want this information, it's easy to get from GitHub. We'll have those warnings again, but they won't fail the build.
It was overzealous of me. YAGNI.
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.
Removed.
These are more maintenance hassle than they are worth. [See explanation](#54 (comment))
Hi Michael, I am still getting warnings on the following: Mac / py3.6 / ciso 2.0.1
|
@liampauling, thanks for letting us know. I don't know how we missed that one. I'm planning to get rid of the As a work-around for your local development, you could remove the compiler flag lines in setup.py. Let me know if this works for you. |
@thomasst, making it fail on compiler warning might have been overzealous. In my mind, the point of this was to help with our vetting of changes made in PRs before they were merged. But since ciso8601 is a source distribution, I fear that we might be stunting adoption if the pip installation fails due to their custom compiler configuration. I wonder if we should back out these changes, or figure out a way to only apply them during our CI builds / tests? Edit: #63 resolves this concern. |
Note that this isn't failing the install just showing me warnings. C compiler? No idea, sorry. |
@liampauling no worries. We haven't published a release since this change was merged. I'm now concerned what would happen if we were to. 😟 Edit: #63 resolves this concern. |
Resolves #52