-
Notifications
You must be signed in to change notification settings - Fork 344
Cleaning up the release scripts #12
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
Conversation
hiranya911
commented
Mar 30, 2017
- Removed verbose comments
- Structured the script to look like https://github.com/google/oauth2client/blob/master/setup.py
@@ -6,6 +6,8 @@ | |||
from firebase_admin import credentials | |||
|
|||
|
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.
Can you add a comment with the link to https://www.python.org/dev/peps/pep-0396/#specification in it so people know why we have this?
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.
Done
setup.py
Outdated
|
||
if sys.version_info < (2, 7): | ||
print('firebase_admin requires python2 version >= 2.7.', file=sys.stderr) |
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 could see someone reading this and assuming we don't support python3. Should we make it clear in this comment that we support both?
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.
Updated the displayed message
'Programming Language :: Python :: 2', | ||
'Programming Language :: Python :: 2.7', | ||
'Programming Language :: Python :: 3', | ||
'Programming Language :: Python :: 3.3', |
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 kind of harkens back to our discussion from Wednesday, but do we need to explicitly list 3.3 and 3.5 here? I'm fine if we want to keep it, but it just seems kinda random.
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 figured we should list the Python versions that we have explicitly tested on. oauth2client does something similar: https://github.com/google/oauth2client/blob/master/setup.py
Updated the comments as suggested. |
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.
LGTM
@@ -6,6 +6,8 @@ | |||
from firebase_admin import credentials | |||
|
|||
|
|||
# Declaring module version as per https://www.python.org/dev/peps/pep-0396/#specification | |||
# Update this accordingly for each release. |
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.
Just a thought: We should probably have a script like we do in Java (release-to-gh.sh
) and Node.js (create-release-tarball.sh
) which takes the version number as an argument and updates this kind of stuff automatically.