-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New explore() function with prompt_toolkit 2.0 integration #1413
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
Conversation
e267193 to
56aee08
Compare
Codecov Report
@@ Coverage Diff @@
## master #1413 +/- ##
==========================================
+ Coverage 85.31% 85.31% +<.01%
==========================================
Files 179 179
Lines 42251 42342 +91
==========================================
+ Hits 36046 36124 +78
- Misses 6205 6218 +13
|
|
I like this (a lot), but I'd rather keep the good old |
|
PR status: waiting for ipython/ipython#11177 then it’s release. I've renamed the function |
|
I messed up my rebase: latest changes are in latest commit :/ |
scapy/packet.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to add this to the docstring (and users will see that using help()). I don't want to be annoyed when I use ls() and know what I'm doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only be printed when the user types ls().
If he uses ls(IP)... or anything similar, it won't be printed.
because ls() result is unreadable, I don't think this really is an issue :/
|
IPython 7 is now live. I’ll rebase this |
694c888 to
cacacd3
Compare
guedou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Seem my comments.
scapy/packet.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/scapy/Scapy/
|
Several comments are trying the PR:
|
|
More comments after using
I am not a big fan of this "GUI" however I really like the idea of exploring layers using a name. It will likely help new comers. |
I thought throwing this error (it's scapy who is doing it) was the clearer way of reporting a missing dependency :/
I manually added Packet to the list, so that
Would that really be useful ? how to specify if its a module or a layer ?
Not yet. That's on prompt_toolkit part. I've already submited a few PRs to improve how the list is handled, and am planning to add a few more (support for pagedown and pageup, maybe search...) |
|
@gpotter2 I prefer displaying an error message in interactive mode, but I can leave without it. The non-GUI version could only work for layers, that is a good enough trade-off. |
|
@guedou So I added a non-GUI version + tests. |
57e7863 to
0723de1
Compare
|
Please ignore codacy: the eval is safe as layers only and the import is required by the eval |
guedou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment. It its good to be merged.
Current ls() function has different features:
ls(IP)ls("IP")ls()The last function,
ls()is currently un-readable and very useless, for several reasons:_)This PR adds a fancy system to discover packets (last case only), when launched in interactive mode, by using the funcionalities of
prompt_toolkit(module included in IPython)Screenshots
Layers
Contribs
Important notes
This PR requiresprompt_toolkit>=2.0.0, the new prompt_toolkit version. It is currently unavailable on PyPi, but according to ipython/ipython#10964, bothprompt_toolkit>=2.0.0andipythonsupporting it should be released very soon (matter of days).Both prompt_toolkit 2.0 and IPython 7 have now been released. The PR is now mergeable