Issue #22: Implement sorting using a struct to hold the state of the sorting #43
Conversation
Picked two arbitrary keybinds "[" toggles on sortedByPercentage and will toggle back and forth between Asc and Desc order. "]" turns off sorting by percentage. Need to add "instructions" to the key map. i.e. "[ toggle sort by percentage"
aidanmatchette
changed the title
There was a problem hiding this comment.
Hi @aidanmatchette,
Thank you! A few things to consider:
- Please use
go fmtto format your code, or configure your editor to run it every time you make changes. It makes all Go code look the same, which improves readability and consistency. - Right now this code cannot be merged, because it doesn't support sorting by name. To make sure it works end to end, we have to implement sorting by name. Since you already have the type for it, and a function that sorts based on percentage, it should be fairly simple to add another mode.
sortByCoverageis not needed anymore, you can replace it completely with sort type.
internal/model/model.go
Outdated
| ASC sortOrder = true | ||
| DSC sortOrder = false |
There was a problem hiding this comment.
No need to make these exported if everything else is not exported (use lowercase letters)
internal/model/model.go
Outdated
| // New create a new model that can be used directly in the tea framework. | ||
| func New(opts ...Option) *Model { | ||
| m := &Model{ | ||
| activeView: activeViewList, | ||
| helpState: helpStateShort, | ||
| sortState: SortState{Type: sortStateByName, Order: ASC}, |
There was a problem hiding this comment.
Nice! Great way of keeping things backwards compatible 😼
internal/model/model.go
Outdated
| @@ -78,9 +99,11 @@ type Model struct { | |||
| detectedPackageName string | |||
| requestedFiles map[string]bool | |||
| filteredLinesByFile map[string][]int | |||
| profilesLoaded []*cover.Profile | |||
There was a problem hiding this comment.
What do you think about calling this coverProfiles or loadedProfiles instead?
internal/model/model.go
Outdated
| //toggle on and insiate sortByCoverage (default = Asc) | ||
| case "s": | ||
| m.toggleSort() | ||
| m.Update(m.profilesLoaded) |
There was a problem hiding this comment.
I think that internal bubbletea methods should not be called by our code; instead, the framework handles these calls externally, and we only need to react to them. Instead, I suggest to implement a new function updateProfilesList or similar, which checks the current sort state (type + direction), figures out the items in the correct order, and sets them into the list (m.list).
The resulting code should look something like this:
case "s":
m.toggleSort()
return m.updateListItems()
case "!":
m.toggleSortOrder()
return m.updateListItems()and updateListItems will sort the list based on the state, covert the sorted profiles to list items, and call m.list.setItems.
There was a problem hiding this comment.
@orlangure After sorting the list items depending on the SortState, does calling the m.list.SetItems automatically update the UI?
There was a problem hiding this comment.
I could use some help and direction on sorting by name. Is it best to sort via m.list or m.loadedProfiles. I apologize, I am new to Golang and I am trying to figure out the best way to do this. Any suggestions are greatly appreciated.
func (m *Model) updateListItems() tea.Cmd {
switch m.sortState.Type {
case sortStateByName:
//sort by name
case sortStateByPercentage:
//sort by percentage
}
return func() tea.Msg {
return m.loadedProfiles
}
}There was a problem hiding this comment.
After sorting the list items depending on the SortState, does calling the m.list.SetItems automatically update the UI?
Yes, if done properly this will cause the UI to redraw. The flow in bubbletea framework works like "action -> model changes -> rendering", and setting list items is the second step - "model changes". Rendering will follow, and will pick up the updated order of items.
Regarding your second question, below is my draft of the changed functions, based on your code:
func (m *Model) updateListItems() (tea.Model, tea.Cmd) {
m.sort()
m.items = make([]list.Item, len(m.profilesLoaded))
for i, p := range m.profilesLoaded {
// package name should already be set
p.FileName = strings.TrimPrefix(p.FileName, m.detectedPackageName+"/")
m.items[i] = &coverProfile{
profile: p,
percentage: percentCovered(p),
}
}
return m, m.list.SetItems(m.items)
}
func (m *Model) sort() {
switch m.sortState.Type {
case sortStateByPercentage:
m.sortByPercentage()
case sortStateByName:
m.sortByName()
}
}
func (m *Model) sortByPercentage() {
sort.Slice(m.profilesLoaded, func(i, j int) bool {
if m.sortState.Order == ASC {
return percentCovered(m.profilesLoaded[i]) < percentCovered(m.profilesLoaded[j])
} else {
return percentCovered(m.profilesLoaded[i]) > percentCovered(m.profilesLoaded[j])
}
})
}
func (m *Model) sortByName() {
sort.Slice(m.profilesLoaded, func(i, j int) bool {
if m.sortState.Order == ASC {
return m.profilesLoaded[i].FileName < m.profilesLoaded[j].FileName
} else {
return m.profilesLoaded[i].FileName > m.profilesLoaded[j].FileName
}
})
}It is not the most efficient and/or elegant way of solving the problem, but it works. It sorts the "loaded profiles" slice, and then loads the already sorted items into the list.
There was a problem hiding this comment.
Great stuff! I was playing around with this and nearly had this same implementation, just couldn't figure out some of the Golang syntax. Thanks for this assistance. How would you feel about pulling out this code block to have its own function. This same process is currently used twice in the code. Ref onProfilesLoaded line 206.
m.items = make([]list.Item, len(m.profilesLoaded))
for i, p := range m.profilesLoaded {
// package name should already be set
p.FileName = strings.TrimPrefix(p.FileName, m.detectedPackageName+"/")
m.items[i] = &coverProfile{
profile: p,
percentage: percentCovered(p),
}
}I will push my current changes and this PR should update so you can see where I am at.
I have created a model_test.go file, but I am not too sure where to start with that.
And then add the button shortcuts to the UI so users will be able to see how to toggle the sort features.
Thanks as always!
Changes have been made to make sortOrder enum non-exported. We are no longer directly calling bubbletea Update function, let the framework handle that by updating m.list. Named changes in some variables and functions for better clarity.
This project uses snapshot tests using "golden" files, see This is a CLI utility, and for me the best way to make sure there are as little regressions as possible, is to "remember" how the output looks like, run the program and make sure the output is not changed. After all, the product of this program is some text that is rendered in the terminal. I use this project for golden file testing: https://github.com/sebdah/goldie, you can play with it a bit. Then, look at the |
|
@orlangure Thanks for that info. I am trying to wrap my head around how the snapshot testing works. I am trying to figure out where to start with this. I could use some direction to get me started on the testing. I have taken a look at goldie and the |
|
Look at this file, this is a set of tests of the happy flow. There are examples of navigation and key presses, you should be able to add another case that will toggle the feature multiple times and assert the output. Note that the first time it will obviously fail, so you will need to accept golden files (see Taskfile). Please make sure that the program looks and behaves exactly like you want it to, and only then "save" this expected state to golden files. |
|
Sorry I have been a bit slow to respond, busy time at work lately. I added testing methods to Thanks as always. |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #43 +/- ##
==========================================
+ Coverage 86.12% 86.42% +0.30%
==========================================
Files 10 10
Lines 807 862 +55
==========================================
+ Hits 695 745 +50
- Misses 94 97 +3
- Partials 18 20 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Hey @aidanmatchette, great progress and definitely the right direction.
Let me know if you need any guidance in the above steps. So far you are doing great, it already is usable and helpful. Let's now improve the UX and ship it 😼 |
|
I'll try to get a PR with some of these changes sometimes this week. Thanks for all your support! |
|
I've adjusted the I also want to sort options to be added to the keymaps at the bottom, e.x. Thanks for all the help!
|
|
I'd use the same color as the percentage coverage of every entry in the list (grey color) in the following format right after the word "Available" in the screenshot: Using this as the starting point, we can try different options. Regarding the keymap, I don't see how it is possible out of the box. To extend the default keymap, we should introduce our own list model that wraps the default list model, and only overrides the keymap method used to render the help. Can you try that and let me know if you get stuck on anything? |
|
I was reading through the bubbletea/bubble help docs to try to add to the
I then added the new keymap to the How did you previously set the keymaps to display in the Thanks for any help or guidance! |
|
The default mappings are set in the "List" component, we don't modify them (currently) and get them for free. That's why I suggested to start using a custom "List" wrapper, that will inherit everything from the regular list, except for the key mappings that we can override. |
|
I am trying to figure this problem out. I was reading through the I don't know if you have already gone down this road or not, but I also will be looking at implementing our own Model instance to have custom keybindings, but this seems like it might be overkill. I would love your input. |
|
Sorry for taking long to reply. I looked into custom key bindings injection into "Help" component of "List" model, and couldn't find a quick and easy solution for that. I suggest that we print inline help in the same place we put sort definition. I think what would be "good enough" is something that looks line this: Available files: %↓ s/! change Percentage sign + ab should make the mode clear, and arrows will show the direction. Just need to come up with a nice style to display this. I'd use default white color bold for mode+direction, dark grey bold for |
Issue #22 : Using your suggestions I have created a struct to store the state of all of the sorting configurations. This will make it easy to add more configurations in the future with minimal changes to the existing code. As of now "s" will toggle through the sort "Type" i.e. (sortByName / sortByPercentage). If sortByPercentage is currently selected, user can toggle the sortOrder i.e (ascending / descending) by using the "!".
Please give me suggestions for improvements and next steps. 😀
Yet to be implemented