various naming utilities for canonicalization and display #570
Conversation
k8s-ci-robot
added
area/multi-index
cncf-cla: yes
k8s-ci-robot
added
approved
cmd/krew/cmd/util_test.go
Outdated
| if !isDefaultIndex("default") { // nb: intentionally not using the const to ensure compatibility | ||
| t.Error("default index must indicate default index") | ||
| } | ||
| if isDefaultIndex("foo") { // nb: intentionally not using the const to ensure compatibility |
There was a problem hiding this comment.
| if isDefaultIndex("foo") { // nb: intentionally not using the const to ensure compatibility | |
| if isDefaultIndex("foo") { |
There was a problem hiding this comment.
Also just for my own clarification, what does nb stand for?
cmd/krew/cmd/util.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package cmd |
There was a problem hiding this comment.
Should this go in cmd/internal? It seems like cmd only has the cobra commands, so it could be misleading at first glance where devs could think there's a krew util subcommand. This is super minor and probably just for my own clarification.
There was a problem hiding this comment.
possibly, though, it's kinda unidiomatic to make util packages for these sort of things. You're right that this almost implies that there's a "util" command; though, do we care about that?
There was a problem hiding this comment.
Might rename to namingutils.go
There was a problem hiding this comment.
I don't think its a big deal, just a small nit from my perspective.
There was a problem hiding this comment.
renamed to namingutils.go
Refactoring some of the utilities from the patch that ported 'krew list' to multi-indexes. These utilities will help us implement 'krew search'. Signed-off-by: Ahmet Alp Balkan <[email protected]>
Signed-off-by: Ahmet Alp Balkan <[email protected]>
ahmetb
force-pushed
the
cmd-naming-utils
branch
from
57fbfb3 to
05a41f0
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
Refactoring some of the utilities from the patch that ported 'krew list'
to multi-indexes.
These utilities will help us implement 'krew search'.
/area multi-index
/assign @corneliusweig
cc: @chriskim06