-
-
Notifications
You must be signed in to change notification settings - Fork 726
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
[cmd] Add support for flags specifying filters type in vmmap, and allow multiple filters #1120
base: main
Are you sure you want to change the base?
Conversation
I will write docs and tests later :P |
No problem, I'll make this a draft then to see the diff with that's ready to review. Change the status when ready |
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.
Interesting addition but I think you should rewrite the parsing loop using the parse_arguments
decorator here. It would gain in simplicity and clarity.
…_arguments` decorator (#1122) Add support for the `append` action for ArgumentParser in the `parse_arguments` decorator on optional arguments. This makes us able to support this: ``` command --arg 1 --arg 2 --arg 3 ``` And get `[1, 2, 3]` as the `arg` value. This CL is required to make changes to #1120 before merging. --------- Co-authored-by: crazy hugsy <hugsy@users.noreply.github.com>
48ada57
to
ec99843
Compare
c8d73cd
to
e9b45bc
Compare
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.
Mostly ok, just needs tests
93f588e
to
3da17fe
Compare
filters = names_filter + addrs_filter | ||
filter_content = f"[{' & '.join(filters)}]" | ||
|
||
if len(names) + len(addrs) == 0: | ||
self.print_entry(entry) |
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 you could continue after this, and then remove the elif
below, & un-indent the rest. Also, couldn't the if
above just be if not filter_content
?
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.
We can't remove the elif because we want to check if there are matched filters (which is not already checked)
And we can't replace the first if with if not filter_content
since we want to check if there are existing filters, but this would check if there are matched filters
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.
ok, then I think if not names and not addrs
is probably clearer
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.
up to you though. Approved.
Co-authored-by: Grazfather <grazfather@gmail.com>
filters = names_filter + addrs_filter | ||
filter_content = f"[{' & '.join(filters)}]" | ||
|
||
if len(names) + len(addrs) == 0: | ||
self.print_entry(entry) |
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.
ok, then I think if not names and not addrs
is probably clearer
Parse args:
-a
/--addr
:-n
/--name
: