-
Notifications
You must be signed in to change notification settings - Fork 125
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
commands: add grep #678
commands: add grep #678
Conversation
40d1670
to
f2b9735
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.
Who is this new mbolivar-ampere guy? :-)
Could you bother adding just ONE, super basic, simple, minimal, incomplete and mostly hardcoded test? It would:
- already provide infinitely more test coverage than no test at all
- get the ball rolling and makes it vastly easier to expand later, especially for people not familiar with west's test suite (= everyone but you).
The perfect is the enemy of the good.
@marc-hb for sure I'll be adding tests before merging. I'll update the --quiet and internal-reminder-when-adding-more-tools as well. Thanks for review -- other comments responded to. |
762e201
to
da08c54
Compare
@marc-hb please revisit |
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.
A few minor suggestions and questions left after a pretty thorough review, everything else LGTM
da08c54
to
4c76e15
Compare
The WestCommand API for running subprocesses is OK, but is missing some useful features; add them now: - The current recommendation in the subprocess module is to use subprocess.run(), so add a helper for doing that since it's missing. (The name asymmetry is because there is already a WestCommand.run() claimed.) - Allow subprocess invocations to take any kwargs we want Signed-off-by: Martí Bolívar <[email protected]>
This is useful when we want to be able to control whether a newline is appended or not. The die() method is intentionally omitted: this is a "scream and exit the process" method, and always flushing any line-buffered I/O makes sense to enforce in this context. Signed-off-by: Martí Bolívar <[email protected]>
This has been requested various times. I haven't done it because 'west forall -c "git grep"' has been a reasonable workaround for me, and I never had time or need to think about what an ergonomic alternative might look like. That's changed now and I wanted something that would only print output for projects where a result was found, similarly to the way "west diff" only prints for projects with nonempty "git diff", etc. Add a "grep" command that does similar, defaulting to use of "git grep" to get the job done. Also support (my favorite) ripgrep and plain "grep --recursive". See the GREP_EPILOG variable in the patch for detailed usage information. Signed-off-by: Martí Bolívar <[email protected]>
Get some minimal coverage. Signed-off-by: Martí Bolívar <[email protected]>
4c76e15
to
da48280
Compare
@marc-hb comments addressed and one additional bug in |
and... it hangs forever :-) Hotfix submitted in #681 Looks like personal configs are still "polluting" the test suite... |
This has been requested various times, but its lack never bothered me up until now, etc. Anyway, here it is.
Fixes: #337