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

combine - a proper python package #138

Merged
merged 4 commits into from May 5, 2015

Conversation

Projects
None yet
3 participants
@ivanlei

ivanlei commented May 3, 2015

This change effectively builds combine as a proper python package.

setup.py

This add a distutils compatible setup.py. combine is now a real and easy to install python package.

  • new code location - The code has moved from the project root to inside the combine/ dir.
  • deal with it version numbers - The version # is in 2 places :( - both in setup.py and in combine/combine.py. Having it in just 1 place would be ideal.
  • console_scripts - From within the virtualenv or if the combine package is installed, there is no need to $ python combine.py anymore. From any directory just use $ combine (or $ reaper, $ thresher, $ winnower, $baler). Magic makes sure the right python code is called.

tox

tox is a super popular way to build virtualenvs and run tests. This change adds a tox.ini that both builds virtualenvs and runs tests.

  • write some tests - There are still no tests!
  • make builds virtualenvs - There's also a Makefile that mostly just calls tox
    • $ make venv - creates a virtualenv in venv-combine. Enter the virtualenv with $ source venv-combine/bin/activate
    • $ make test - creates a virtualenv in .tox, runs all the tests, runs code coverage, and exits.
  • grequests - Getting grequests to properly install whether manually running $ pip install -r requirements.txt or $make test took some doing.
    • Passing --allow-external grequests to pip install was necessary.
    • Somehow that was enough to let stuff run when installing manually but not when installing with tox. Turns out the -e . param in requirements-dev.txt seems to not inherit --allow-external so I used -r requirements.txt instead which magically worked. I only wasted like 90 min figuring that out.

pre-commit

pre-commit is a simple tool for ensuring code quality and finding bugs. It adds git hooks that validate the code. After building a virtualenv run $ pre-commit install

docker

These changes have been tested in docker. The Dockerfile and docker readme have been updated. The existing docker stuff was broken prior to these changes.

functional code changes

There are very few functional code changes

  • argparse - Argument parsing has been improved in combine.py
    • --version has been added.
    • --output-dir has been added.
    • minor cleanup of code for existing arguments.
@alexcpsec

This comment has been minimized.

Show comment
Hide comment
@alexcpsec

alexcpsec May 3, 2015

Member

Wow! ❤️

Member

alexcpsec commented May 3, 2015

Wow! ❤️

@alexcpsec

This comment has been minimized.

Show comment
Hide comment
@alexcpsec

alexcpsec May 3, 2015

Member

Need to send CLA.

Member

alexcpsec commented May 3, 2015

Need to send CLA.

@krmaxwell

This comment has been minimized.

Show comment
Hide comment
@krmaxwell

krmaxwell May 4, 2015

Member

I will be reviewing this in the next day or so, but let me know about the CLA @alexcpsec.

And thanks @ivanlei !! 🙇

Member

krmaxwell commented May 4, 2015

I will be reviewing this in the next day or so, but let me know about the CLA @alexcpsec.

And thanks @ivanlei !! 🙇

@alexcpsec

This comment has been minimized.

Show comment
Hide comment
@alexcpsec

alexcpsec May 4, 2015

Member

CLA is signed

On Sun, May 3, 2015 at 5:29 PM, Kyle Maxwell notifications@github.com
wrote:

I will be reviewing this in the next day or so, but let me know about the CLA @alexcpsec.

And thanks @ivanlei !! 🙇

Reply to this email directly or view it on GitHub:

#138 (comment)


This e-mail message and any files transmitted with it contain legally
privileged, proprietary information, and/or confidential information,
therefore, the recipient is hereby notified that any unauthorized
dissemination, distribution or copying is strictly prohibited. If you have
received this e-mail message inappropriately or accidentally, please notify
the sender and delete it from your computer immediately.

Member

alexcpsec commented May 4, 2015

CLA is signed

On Sun, May 3, 2015 at 5:29 PM, Kyle Maxwell notifications@github.com
wrote:

I will be reviewing this in the next day or so, but let me know about the CLA @alexcpsec.

And thanks @ivanlei !! 🙇

Reply to this email directly or view it on GitHub:

#138 (comment)


This e-mail message and any files transmitted with it contain legally
privileged, proprietary information, and/or confidential information,
therefore, the recipient is hereby notified that any unauthorized
dissemination, distribution or copying is strictly prohibited. If you have
received this e-mail message inappropriately or accidentally, please notify
the sender and delete it from your computer immediately.

@krmaxwell krmaxwell self-assigned this May 5, 2015

@krmaxwell krmaxwell added this to the v0.2.0 Dingo milestone May 5, 2015

@krmaxwell krmaxwell merged commit 398b25f into mlsecproject:dev May 5, 2015

@krmaxwell krmaxwell removed the in progress label May 5, 2015

@krmaxwell

This comment has been minimized.

Show comment
Hide comment
@krmaxwell

krmaxwell May 5, 2015

Member

Thanks, this was fantastic. I'd started to work locally on a package, so you saved me a lot of work (not to mention all the other useful stuff here!)

Member

krmaxwell commented May 5, 2015

Thanks, this was fantastic. I'd started to work locally on a package, so you saved me a lot of work (not to mention all the other useful stuff here!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment