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

Added dictionary functionality to Rule(6) class #222

Merged
merged 4 commits into from
Feb 18, 2019
Merged

Conversation

jllorente
Copy link
Collaborator

This commit adds the dictionary functionality to Rule() and Rule6() classes.
This addresses a previous feature request reported in issue #219

@ldx
Copy link
Owner

ldx commented Nov 27, 2017

Did you try running tests locally? They failed on Travis:

test_hashlimit (tests.test_matches.TestHashlimitMatch) ... 
travis_time:end:054ea712:start=1510535572443631907,finish=1510535573155267335,duration=711635428
�[0K
�[31;1mThe command "sudo PATH=$PATH coverage run setup.py test" exited with 139.�[0m

Done. Your build exited with 1.

@jllorente
Copy link
Collaborator Author

I ran the test on my system for both python2 (2.7.12) and python3 (3.5.2). They ran ok, all of them :)

~/python-iptables/tests# $PYTHON ./test_iptc.py
~/python-iptables/tests# $PYTHON ./test_matches.py
~/python-iptables/tests# $PYTHON ./test_targets.py

@ldx
Copy link
Owner

ldx commented Dec 20, 2017

Sorry for the delay on my part, I'd like to look into the failures in the Travis CI build. They look like segfaults when running the test suite.

@coveralls
Copy link

coveralls commented Mar 19, 2018

Coverage Status

Coverage increased (+0.2%) to 59.5% when pulling edabe7c on jllorente:master into 583f939 on ldx:master.

@jllorente
Copy link
Collaborator Author

It seems the build issues have been resolved and now there are no conflicts. I have also improved the dictionary generation function to remove empty lists and fields.

@ldx
Copy link
Owner

ldx commented Mar 19, 2018

Nice! Can you also add tests for the dict methods, and a few lines to doc/examples.rst? Thanks!

@jllorente
Copy link
Collaborator Author

I completely forgot about this PR, sorry about that!
Can we merge as is ?

Copy link
Owner

@ldx ldx left a comment

Choose a reason for hiding this comment

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

Nice 👍

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.

None yet

3 participants