Skip to content
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

Python 3 + Mixbox Support #249

Merged
merged 17 commits into from
May 28, 2015
Merged

Python 3 + Mixbox Support #249

merged 17 commits into from
May 28, 2015

Conversation

gtback
Copy link
Contributor

@gtback gtback commented May 14, 2015

This obsoletes #228 and incorporates mixbox, with the common "binding utils" delegated to that package. Mixbox natively supports Python 3, and this branch has also been updated to support Python 3.2, 3.3, and 3.4.

This requires mixbox 0.0.2. I didn't explicitly specify a version, since I don't want to remember to bump it each time in several places. If you have a different version of mixbox cached locally with pip, you can just "pip install mixbox==0.0.2" from any virtualenv to download and cache the newest version, which will get used to build new venvs.

@gtback gtback mentioned this pull request May 14, 2015
@landscape-bot
Copy link

Code Health
Repository health increased by 0.03% when pulling 9210af1 on mixbox-rebased into 85bc959 on master.

@gtback
Copy link
Contributor Author

gtback commented May 20, 2015

@bworrell, I know this is a huge change, but do you have time to look at it and provide any feedback?

@bworrell bworrell self-assigned this May 26, 2015
@@ -1523,7 +1522,7 @@ def buildChildren(self, child_, node, nodeName_, fromsubclass_=False):
'Fuzzy_Hash_Value': cybox_common.FuzzyHashValueType,
'Show_Message_Title': cybox_common.StringObjectPropertyType,
'Data_Size': cybox_common.DataSizeType,
'Email_Message': email_message_object.EmailMessageObjectType,
'email_message': email_message_object.EmailMessageObjectType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a find-replace gone bad. It pops up in a couple spots. I fixed it in 8327de6.

@bworrell
Copy link
Contributor

@gtback, I looked through the code and posted a few comments/questions but overall I think it looks awesome!

I ran nosetests against my local python-stix clone and it failed because ExternalEncoding no longer lives in cybox.bindings. If I add the following line to __init__.py in cybox.bindings, everything passes:

# For python-stix/python-maec
from mixbox.binding_utils import ExternalEncoding

I'm not sure if we want to add the line, or just work towards getting python-stix|maec up and running with mixbox before pushing out a new release of python-cybox. What do you think?

@gtback
Copy link
Contributor Author

gtback commented May 28, 2015

I would lean towards not adding to cybox.bindings.__init__, since the python-stix|maec code doesn't actually do what it's supposed to after that change (it redefines cybox.bindings.ExternalEncoding, but that doesn't actually modify mixbox.binding_utils.ExternalEncoding which is what the CybOX bindings use for output). Better to just update python-stix|maec quickly. I don't see an immediate need for a python-cybox release.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.03% when pulling 603f7a9 on mixbox-rebased into 85bc959 on master.

@bworrell
Copy link
Contributor

@gtback, this looks awesome! I'm gonna merge, though we'll need to update python-stix and python-maec before the next release. Thanks!!

bworrell pushed a commit that referenced this pull request May 28, 2015
@bworrell bworrell merged commit 1b38e12 into master May 28, 2015
@bworrell bworrell deleted the mixbox-rebased branch May 28, 2015 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants