Skip to content
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

✨ make components.Select() generic #4811

Merged
merged 2 commits into from
Nov 4, 2024
Merged

✨ make components.Select() generic #4811

merged 2 commits into from
Nov 4, 2024

Conversation

afiune
Copy link
Contributor

@afiune afiune commented Nov 4, 2024

This change is adding a new SelectableItem interface that the function components.Select()
accepts so that we can select any type of slice.

Additionally, I improved the components.List() function to use the tabwriter when
running things without a TTY.

Finally, I added the following README.md


components package

This Go package has interactive helpers used by cnquery and cnspec.

We use a powerful little TUI framework called bubbletea.

Select component

Select is an interactive prompt that displays the provided message and displays a
list of items to be selected.

e.g.

type CustomString string

func (s CustomString) HumanName() string {
	return string(s)
}

func main() {
	customStrings := []CustomString{"first", "second", "third"}
	selected := components.Select("Choose a string", customStrings)
	fmt.Printf("You chose the %s string.\n", customStrings[selected])
}

To execute this example:

go run cli/components/_examples/selector/main.go

List component

List is a non-interactive function that lists items to the user.

e.g.

type CustomString string

func (s CustomString) PrintableKeys() []string {
	return []string{"string"}
}
func (s CustomString) PrintableValue(_ int) string {
	return string(s)
}

func main() {
	customStrings := []CustomString{"first", "second", "third"}
	list := components.List(theme.OperatingSystemTheme, customStrings)
	fmt.Printf(list)
}

To execute this example:

go run cli/components/_examples/rawlist/main.go

Copy link
Contributor

github-actions bot commented Nov 4, 2024

Test Results

3 131 tests  ±0   3 130 ✅ ±0   1m 53s ⏱️ +24s
  371 suites ±0       1 💤 ±0 
   28 files   ±0       0 ❌ ±0 

Results for commit b311ed1. ± Comparison against base commit 8e42171.

♻️ This comment has been updated with latest results.

This change is adding a new `Selector` interface that the function
`components.Select()` accepts so that we can select any type of slice.

Additionally, I improved the `components.List()` function to use the
`tabwriter` when running things without a TTY.

Signed-off-by: Salim Afiune Maya <afiune@mondoo.com>
@afiune afiune force-pushed the afiune/integrate-cmd branch from 3fb89cb to bc63e5c Compare November 4, 2024 10:53
Copy link
Member

Choose a reason for hiding this comment

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

why not have this added to a readme or some other development doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea! I can add some docs.

This example is useful to test this interactive code, you can run it with

$ go run cli/components/_examples/selector/main.go

I'll add it to the docs. 👍🏽

)

// Object is the interface that a list need to implement so we can display its objects.
type Object interface {
Copy link
Member

Choose a reason for hiding this comment

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

can we have a more specific name for this interface? I find this one too generic making it unclear what it's purpose is. Perhaps call it CliListableItem or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change. 👍🏽

b := &strings.Builder{}
w := tabwriter.NewWriter(b, 1, 1, 1, ' ', tabwriter.TabIndent)

log.Debug().Msgf("discovered %d %s(s)", len(list), listType)
Copy link
Member

Choose a reason for hiding this comment

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

i think this message makes no sense here. The discovery mechanism is related to asset discovery when starting a scan. You are building a generic function that can be used for any type of object (not necessarily an asset).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch!


// Selector is the interface that items need to implement so that we can select them.
type Selector interface {
HumanName() string
Copy link
Member

Choose a reason for hiding this comment

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

nit: weird name. Perhaps call it FriendlyName or similar.
Would be nice to add documentation what this function is used for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to re-implement the function for the assets, since it already exists and, it is the only place we use it (for now).

If we are ok with a new function, then I'll go with Display()

)

// Selector is the interface that items need to implement so that we can select them.
type Selector interface {
Copy link
Member

Choose a reason for hiding this comment

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

this one also seems to generic. CliSelectableItem maybe?

Copy link
Contributor Author

@afiune afiune Nov 4, 2024

Choose a reason for hiding this comment

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

Sure thing.

for i := range list {
assetObj := list[i]

for i, key := range assetObj.PrintableKeys() {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I am in favour of this construction. We are building a generic way of displaying lists of items and selecting an item from that list. I think the way an item from the list is displayed, should be defined outside of this function. The current implementation seems like it provides some "customization" options but it is actually very limited.

If you don't need the keys and values individually, why not just make the interface for Object have a function Print() string or something similar, and just call this here. Then it is up to the Object to specify how it is printed on the CLI. We also remove the PrintableKeys and PrintableValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree.

Leaving the formatting to the implementation (caller) is not an option, if we want that, we should not have this function at all.

The whole point of these helpers is to help and standardize how we print/interact with "stuff".

This comment has been minimized.

@afiune afiune force-pushed the afiune/integrate-cmd branch from 85cfd01 to 87732d9 Compare November 4, 2024 12:39
Signed-off-by: Salim Afiune Maya <afiune@mondoo.com>
@afiune afiune force-pushed the afiune/integrate-cmd branch from 87732d9 to b311ed1 Compare November 4, 2024 12:42
@afiune afiune requested a review from imilchev November 4, 2024 12:46
@afiune afiune merged commit d69318b into main Nov 4, 2024
15 checks passed
@afiune afiune deleted the afiune/integrate-cmd branch November 4, 2024 15:13
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants