Skip to content

feat: coder ls should show possible columns to filter by #4240

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

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

endingwithali
Copy link
Contributor

Solves feature request for #1432

@endingwithali endingwithali self-assigned this Sep 28, 2022
@endingwithali endingwithali added the cli Area: CLI label Sep 28, 2022
@endingwithali endingwithali requested a review from a team September 28, 2022 14:37
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paired with Ali on this. The Reflect() piece is just to get the struct fields on the workspace.
Requesting other eyes but we both tested this together and it works fine from what I can tell.

Comment on lines +88 to +94
if me {
myUser, err := client.User(cmd.Context(), codersdk.Me)
if err != nil {
return err
}
filter.Owner = myUser.Username
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: this might be a left-over piece of a merge conflict

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but it'd be better if it were a set of reusable functions instead :)

Comment on lines +131 to +139
v := reflect.Indirect(reflect.ValueOf(displayWorkspaces))
availColumns, err := cliui.TypeToTableHeaders(v.Type().Elem())
if err != nil {
panic(err)
}
for i, s := range availColumns {
availColumns[i] = strings.Replace(s, " ", "_", -1)
}
columnString := strings.Join(availColumns[:], ", ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but since we'd probably want to use this in multiple commands it would be better served as a function in the cliui package that accepts any:

func TableHeaders(t any) ([]string, error) {
	v := reflect.Indirect(reflect.ValueOf(t))
	return cliui.TypeToTableHeaders(v.Type())
}

func FormatColumnNames(columns []string) string {
	out := strings.Join(columns[:], ", ")
	out = strings.ReplaceAll(out, " ", "_")
	return out
}

which could be called like

columns, err := cliui.TableHeaders(workspaceListRow{})
if err != nil {
	panic(err)
}

columnString := cliui.FormatColumnNames(columns)

This would also remove the need for having a global(ish) var for the output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future fix ideaaa :D

@endingwithali endingwithali merged commit 0c75ea6 into main Sep 28, 2022
@endingwithali endingwithali deleted the endingwithali/column-search-feature branch September 28, 2022 20:39
johnstcn added a commit that referenced this pull request Sep 29, 2022
johnstcn added a commit that referenced this pull request Sep 29, 2022
johnstcn added a commit that referenced this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants