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

Miscellaneous usability improvements #225

Closed
wants to merge 9 commits into from
Closed

Conversation

jaseg
Copy link

@jaseg jaseg commented Jan 28, 2018

Here's three small changes I found useful. Tell me what you think.

First, using @functools.wraps, docstrings and function signatures can be preserved when using @autocommit. This is useful in interactive usage. This change should be backwards-compatible to at least python 2.7.

Second, I found Table.chains not very useful as a plain list, since I'd only iterate over it to get a particular chain anyway. A dict makes more sense to me here. This is obviously breaking the API, so I don't expect you to merge this right away.

Third, I wrote some nice __repr__s for the most common ip4tables classes. This also is really useful in interactive usage, or even when just printing debug output. Be aware that as-is the formatting code pretty-prints IP adresses using the ipaddress module found in python 3.3 upwards.

Thank you for the nice module, jaseg

Since chains are normally accessed using names, a dict is more useful
than a list here.

This change breaks API compatibility.
Add __repr__ for Table, Chain, Rule, Matcher and Target. This greatly
eases debugging and interactive usage.

This change requires at least Python version 3.3 for the ipaddress
module.
@jaseg
Copy link
Author

jaseg commented Jan 28, 2018

I'm not sure what's up with the Travis tests here. All unit tests pass on my machine, and the error it throws looks like it might be unrelated to my changes?

@ldx
Copy link
Owner

ldx commented Jan 29, 2018

I think the tests fail due to the new dependency. Can you try applying this diff:

diff --git a/setup.py b/setup.py
index 145f0c3..f40a9d9 100644
--- a/setup.py
+++ b/setup.py
@@ -23,6 +23,7 @@ setup(
     ext_modules=[Extension("libxtwrapper",
                            ["libxtwrapper/wrapper.c"])],
     test_suite="tests",
+    install_requires=["ipaddress"],
     classifiers=[
         "Development Status :: 5 - Production/Stable",
         "Environment :: Console",

This is necessary for python versions <3.3
@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.3%) to 59.606% when pulling fdf0a0b on jaseg:master into 583f939 on ldx:master.

@jaseg
Copy link
Author

jaseg commented Jan 29, 2018

That was it. I have added a version check, since ipaddress is a built-in module starting in python 3.3. I think it is better to not have that dependency on these versions.

@ldx
Copy link
Owner

ldx commented Jan 29, 2018

Looks good, thanks!

Can you add a few test cases for testing the new __repr__ methods?

@jaseg
Copy link
Author

jaseg commented Jan 30, 2018

Here you go.

Two observations:

  • Travis does not actually run the unittests with the python 3.4 it installed. Instead they're run with python2.7 as evidenced here: https://travis-ci.org/ldx/python-iptables/jobs/335130444#L549
  • There is an upstream bug in the ipaddress module, as the failure we can observe there does not happen on the built-in version of python 3.5.4. I'll see what we can do there. Maybe I'll just remove the fancy formatting.

@jaseg
Copy link
Author

jaseg commented Jan 30, 2018

Turns out, ipaddress is actually behaving to its spec, that spec is just very dumb. I'll just rip out that part of the repr code.

* Use @Property and its .setter decorator.
* Add fancy netmask formatting for src/dst: You can now get 127.0.0.1/24
  instead of 127.0.0.1/255.255.255.0 by using .src_numeric_mask instead
  of just .src.
* Simplify code of .src and dst a bit
@ldx
Copy link
Owner

ldx commented Jan 31, 2018

@jaseg thanks, please let me know once it's ready for review. 👍

@jaseg
Copy link
Author

jaseg commented Feb 1, 2018

There. In my enthusiasm I changed quite a lot, have a look whether that works for you.

@ldx
Copy link
Owner

ldx commented Feb 1, 2018

LGTM, and the tests are passing. The only thing remaining is to update doc/examples.rst - can you look into that as well? Thanks!

@Troyhy
Copy link

Troyhy commented Jan 29, 2021

Is this going to be merged? I've been waiting for a while :)

@ldx
Copy link
Owner

ldx commented Jan 29, 2021

@Troyhy if you (or someone else) can add a few lines of documentation to doc/ and resolve the merge conflicts, I'm happy to merge it :)

@Troyhy
Copy link

Troyhy commented Jan 30, 2021

Ok, I'm using your library in one project. I will try to resolve it as part of the project.

@ldx
Copy link
Owner

ldx commented Jan 18, 2023

Looks stale, closing. Feel free to reopen if this is still relevant.

@ldx ldx closed this Jan 18, 2023
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.

4 participants