Setup file for building distributions and tox integration for testing by hiranya911 · Pull Request #3 · firebase/firebase-admin-python

hiranya911 · 2017-03-23T20:54:19Z

setup.py enables building releases
tox integration supports testing on different platforms (currently only one platform defined -- python 2.7; more platforms will be introduced in future PRs)

… distributions; Added contribution guide

jwngr

I had a handful of mostly minor comments.

jwngr · 2017-03-23T22:06:46Z

.github/CONTRIBUTING.md

+which just ask about usage will be closed. Here are some resources to get help:
+
+- Go through the [guides](https://firebase.google.com/docs/admin/setup/)
+- Read the full [API reference](https://firebase.google.com/docs/reference/admin/node/)


s/node/python

jwngr · 2017-03-23T22:08:03Z

.github/CONTRIBUTING.md

+```bash
+$ git clone https://github.com/firebase/firebase-admin-python.git
+$ cd firebase-admin-python    # go to the firebase-admin-python directory
+$ pip install -U pytest       # globally install pytest test framework and executor


Isn't there such thing as a requirements.txt file that we can use like a package.json so users can just do something like pip install?

Turns out there is. Done.

jwngr · 2017-03-23T22:08:45Z

.github/CONTRIBUTING.md

+higher using pip:
+
+```
+pip install -U pylint


This is a bit redundant given the previous section.

Updated the setup instructions to install the proper versions of the tools.

jwngr · 2017-03-23T22:09:03Z

.github/CONTRIBUTING.md

+unit tests. Download pytest 3.0.6 or higher using pip:
+
+```
+pip install -U pytest


Ditto on this being redundant.

jwngr · 2017-03-23T22:09:20Z

.github/CONTRIBUTING.md

+```
+pytest
+```
+Refer to the pytest [usage and invocations guide](http://doc.pytest.org/en/latest/usage.html)


Nit: add a new line after the triple backticks.

jwngr · 2017-03-23T22:12:52Z

setup.py

+here = path.abspath(path.dirname(__file__))
+
+long_description = ('The Firebase Admin Python SDK enables server-side (backend) Python developers '
+                    'to integrate Firebase into their services and applications. Currently this '


I'd drop the last sentence entirely.

jwngr · 2017-03-23T22:13:27Z

setup.py

+    # Versions should comply with PEP440.  For a discussion on single-sourcing
+    # the version across setup.py and the project code, see
+    # https://packaging.python.org/en/latest/single_source_version.html
+    version='0.0.1',


Fine as is for now, but our first release should be 1.0.0.

jwngr · 2017-03-23T22:14:00Z

setup.py

+    description='Firebase Admin Python SDK',
+    long_description=long_description,
+
+    url='https://github.com/firebase/firebase-admin-python',


Since this won't be open source at launch, I think this should link to https://firebase.google.com/docs/admin/setup/ for now.

jwngr · 2017-03-23T22:15:23Z

setup.py

+    url='https://github.com/firebase/firebase-admin-python',
+
+    # Author details
+    author='Firebase Team',


I'd follow what we do in our other SDKs (e.g. here). Namely, use just "Firebase", no email (unless it's actually required), and the base website URL if that's an option.

Done. No possibility of adding the base URL unfortunately.

jwngr · 2017-03-23T22:16:02Z

setup.py

+        #   3 - Alpha
+        #   4 - Beta
+        #   5 - Production/Stable
+        'Development Status :: 3 - Alpha',


I'm not really sure what this is, but this SDK will not be in Alpha. It will be production / stable, so I'd put 5 here.

Will update it to 5, along with the version number when we are at our first release.

…mentation

hiranya911 · 2017-03-23T23:46:09Z

Made the suggested changes.

jwngr · 2017-03-25T00:55:31Z

.github/CONTRIBUTING.md

+```bash
+$ git clone https://github.com/firebase/firebase-admin-python.git
+$ cd firebase-admin-python                 # go to the firebase-admin-python directory
+$ pip install -r .github/requirements.txt  # Install additional tools and dependencies


I think requirements.txt should live in the root of the folder. .github is a GitHub-only thing and I see requirements.txt as equivalent to package.json.

jwngr · 2017-03-25T00:56:08Z

.github/CONTRIBUTING.md

+
+Here are some highlights of the directory structure and notable source files
+
+* `firebase/` - Source directory for the `firebase` module.


Fair enough. SGTM.

jwngr · 2017-03-25T00:56:35Z

.github/CONTRIBUTING.md

+* `lint.sh` - Runs pylint to check for code quality.
+* `.pylintrc` - Default configuration for pylint.
+* `setup.py` - Python setup script for building distribution artifacts.
+* `tox.ini` - Tox configuration for running tests on different environments.


Please add a line explaining requirements.txt here once you move it into root.

hiranya911 · 2017-03-25T19:28:12Z

Applied suggested changes

jwngr

LGTM!

# This is the 1st commit message: Rename multi_factor_config_mgt.py to multi_factor_config.py # This is the commit message #2: changing name from multi_factor_config_mgt to multi_factor_config # This is the commit message #3: Revert "changing name from multi_factor_config_mgt to multi_factor_config" This reverts commit 45cd3e5.

Added initial tox configuration; Added setup.py for packaging release…

d3dedb3

… distributions; Added contribution guide

hiranya911 requested a review from jwngr March 23, 2017 20:54

hiranya911 assigned jwngr Mar 23, 2017

Hiranya Jayathilaka added 2 commits March 23, 2017 14:14

Updated licensing information (removing OSS license info temporarily)

ea16d3d

Updated project URL

d0b5666

jwngr suggested changes Mar 23, 2017

View reviewed changes

Hiranya Jayathilaka added 4 commits March 23, 2017 16:29

Added requirements file for local setup; Updates to setup.py and docu…

f98d553

…mentation

Added tox to the list of tooling requirements

eb9bb8f

Updated title

e046db3

Added more links to referred files

e846bb8

Setting minimal version for tox

f862f45

jwngr suggested changes Mar 25, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 25, 2017

hiranya911 added 2 commits March 25, 2017 12:11

Moved requirements.txt to the root of the repo

0673854

Added some info on installing pip

e08c62f

hiranya911 assigned jwngr and unassigned hiranya911 Mar 25, 2017

jwngr approved these changes Mar 27, 2017

View reviewed changes

jwngr assigned hiranya911 and unassigned jwngr Mar 27, 2017

hiranya911 merged commit 6e26886 into master Mar 27, 2017

hiranya911 deleted the hkj-packaging-setup-2 branch March 27, 2017 18:48

ifielker added a commit that referenced this pull request Jan 30, 2020

review suggestions #3

ae9527f


		Here are some highlights of the directory structure and notable source files

		* `firebase/` - Source directory for the `firebase` module.

Conversation

hiranya911 commented Mar 23, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Mar 25, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants