-
Notifications
You must be signed in to change notification settings - Fork 363
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
various naming utilities for canonicalization and display #570
Conversation
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.
One trivial nit, otherwise looks great!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isDefaultIndex("foo") { // nb: intentionally not using the const to ensure compatibility | |
if isDefaultIndex("foo") { |
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.
Also just for my own clarification, what does nb
stand for?
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.
nota bene
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might rename to namingutils.go
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 don't think its a big deal, just a small nit from my perspective.
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.
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 <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
57fbfb3
to
05a41f0
Compare
/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