-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve the shell completion api #8346
Improve the shell completion api #8346
Conversation
Basically typing |
LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@edsantiago PTAL |
0895cc0
to
63b1b2f
Compare
I pushed a small update to handle the |
63b1b2f
to
fc498e2
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.
The refactoring LGTM - I'm finding situations where it doesn't work as I expected (podman top
, podman container inspect -l
) but it's still sooooo much better than what we had before. This can be refined over time. Thanks for your initiative in taking this on.
Two very minor suggestions, left up to your discretion.
One main advantage of the new shell completion logic is that we can easly parse flags and adjust based on the given flags the suggestions. For example some commands accept the `--latest` flag only if no arguments are given. This commit implements this logic in a simple maintainable way since it reuses the already existing `Args` function in the cmd struct. I also refactored the `getXXX` function to match based on the namei/id which could speed up the shell completion with many containers, images, etc... I also added the degraded status to the valid pod status filters which was implemented in containers#8081. Signed-off-by: Paul Holzinger <[email protected]>
fc498e2
to
cf4967d
Compare
@edsantiago Updated. My error parsing didn't really work as expected. I have to clean the differences in the error message first. I thought |
Not all commands use a meaningful I have to go through them one by one to see were I can improve this. |
Yes, that makes sense, and it can be fixed over time. No rush, this is already very helpful. |
The tight coupling to (and massaging of) cobra messages bothers me a little, but I can't think of a better way to do it. On the whole this LGTM but I'd like confirmation review from at least one more @containers/podman-maintainers team member. |
/lgtm |
One main advantage of the new shell completion logic is that
we can easly parse flags and adjust based on the given flags
the suggestions. For example some commands accept the
--latest
flag only if no arguments are given.This commit implements this logic in a simple maintainable way
since it reuses the already existing
Args
function in thecmd struct.
I also refactored the
getXXX
function to match based on thenamei/id which could speed up the shell completion with many
containers, images, etc...
I also added the degraded status to the valid pod status
filters which was implemented in #8081.