-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add grep_pipe #130
Add grep_pipe #130
Conversation
aexpect/ops_linux.py
Outdated
""" | ||
Invoke ``grep`` remotely searching for an expression in a command output. | ||
|
||
:param session: vm session |
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.
:param session: session to run the command on
would be more consistent
aexpect/ops_linux.py
Outdated
|
||
:param session: vm session | ||
:type session: ShellSession | ||
:param str cmd: command that its output will be grepped |
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'm not native but this sounds strange. How about command whose output will be grepped
?
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.
Much better, I'm not native either you can see that.
aexpect/ops_linux.py
Outdated
:param session: vm session | ||
:type session: ShellSession | ||
:param str cmd: command that its output will be grepped | ||
:param str pattern: pattern to grep |
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.
Could you please unify this with the grep
param above? (expr
rather than pattern
)
:param str cmd: command that its output will be grepped | ||
:param str pattern: pattern to grep | ||
:param str flags: extra flags passed to ``grep`` on the command line | ||
:param bool check: whether to quietly run grep for a boolean check |
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.
Again, it'd be nice to unify this with the existing grep
by changing the position of check
and flags
.
Something like grep_pipe(session, cmd, expr, check, flags)
would be IMO better (or even session, expr, cmd, check, flags
but logically the cmd
should be first here. Not a strong preference over those two, though.)
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.
Hello @gbladislau, glad to see a new face in aexpect. Is this a theoretical improvement or are you using this somewhere? I'm not sure how important this is since one can simply use session.cmd_status("cmd | grep ...")
manually but if there is a project using this frequently, we can add this as long as it's consistent with the existing grep
command (arguments order and docstrings).
Hi @ldoktor I just saw your review. I use the avocado project on a daily basis at the company I work for. This is just a addition to the project since I used |
2abe149
to
02d1683
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.
Thanks, this looks good. Could you please squash it into a single commit so we can get this in?
This adds a new functionality to the ops_linux.py enabling greping on direct commands outputs.
02d1683
to
1f425a1
Compare
Done! |
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.
thanks, lgtm
This adds a new functionality to the ops_linux.py enabling greping on direct commands outputs.