Beautify lookup after #1505 #1559
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac 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 |
libimage/filters.go
Outdated
| func filterDigest(value string) (filterFunc, error) { | ||
| d, err := digest.Parse(value) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Can you add a test for this case?
There was a problem hiding this comment.
Not sure if we need to add some context to the error. "invalid digest filter: %v"?
There was a problem hiding this comment.
Thanks, fixed both.
This causes an immediate failure on invalid values, instead of silently not matching anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior, both callers now have a value of that type. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Scary features should have scary names. Also add a comment to make it less likely that this semantics will spread. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that early exits are colocated. Should not change behavior, reference.TrimNamed() updating "name" should not change the IsShortName value. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…poAndTag Right now that's not simpler, but it will enable simplification of the caller. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We don't need a reference.NamedTagged now. That also makes the namedTagged variable in the caller more local. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
(BTW, @vrothberg , thank you very much for adding all the unit tests, they made it it possible to refactor with a lot of confidence.) |
|
/lgtm |
Glad they were of use :) I think we owe these tests to your thorough reviews on the other PR. |
This is a set of small cleanups on top of #1505. Apart from the first commit, they should not change behavior, and are not worth backporting.
See individual commit messages for details.