-
Notifications
You must be signed in to change notification settings - Fork 0
bubbletea
: adds bubbletea table component and prepares Update
method for supporting multi models.
#18
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
Conversation
I'm getting to work. See my steps and track my cost here.
|
Thanks for working on this PR! I've reviewed the changes to implement the bubbletea table component. The changes look good - you've properly implemented the A few observations:
Overall, the implementation is clean and follows the established patterns in the codebase. There are no tests specifically for the |
bubbletea/tablemodel
: adds bubbletea table component.bubbletea/tablemodel
: adds bubbletea table component and prepares Update
method for supporting multi models.
bubbletea/tablemodel
: adds bubbletea table component and prepares Update
method for supporting multi models.bubbletea
: adds bubbletea table component and prepares Update
method for supporting multi models.
@MentatBot could you give a second review about the last batch of commits send ? |
Thanks for the additional changes to the PR! I've reviewed the latest commits and I'm impressed with the architectural improvements you've made. The changes go beyond just adding the TableModel implementation to significantly improve the bubbletea component architecture: Architecture Improvements:
TableModel Implementation:
Test Updates:
All of these changes work together to create a more robust architecture that handles state management properly. The pointer receiver pattern for Update() methods is especially important for maintaining state across updates. The tests are passing and the CLI is working as shown in your test results. This is a significant improvement to the codebase, and the TableModel implementation is structured well to allow for future expansion. |
// table is the `bubbletea` table model. | ||
// TODO(@chris-ramon): Wire to the `TableModel` component. | ||
table table.Model | ||
|
||
// baseStyle is the base styling for the `table` model. | ||
baseStyle lipgloss.Style | ||
} | ||
|
||
// `Init` is the `TableModel` method required for implementing the `Model` interface. |
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.
The TableModel.Update method should use a pointer receiver and return (Model, tea.Cmd) to match the Model interface signature and ensure state is properly maintained (like the changes made to ChoicesModel.Update).
// `Init` is the `TableModel` method required for implementing the `Model` interface. | |
func (tm *TableModel) Update(msg tea.Msg) (Model, tea.Cmd) { //nolint:golint,ireturn |
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.
@MentatBot nice catch ! - since currently we are not using TableModel
I'll go ahead and update it in a follow-up PR, I've created an issue for addressing this: #19.
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.
That sounds like a reasonable approach! Thanks for creating an issue to track this. Addressing it in a follow-up PR makes sense, especially since the TableModel isn't currently in active use. The current PR already makes significant architectural improvements to the codebase.
@@ -84,7 +84,18 @@ type TableModelParams struct { | |||
|
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.
For consistency with the other methods in TableModel and to ensure proper state management, it would be good to use a pointer receiver for the Run method as well.
func (tm *TableModel) Run(model any) (any, error) { |
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.
@MentatBot nice suggestion - I've added this to the follow-up PR issue: #19, I'll consider addressing this.
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.
Thanks for adding this to the follow-up issue as well. Both suggestions relate to the same architectural pattern of using pointer receivers consistently, so addressing them together makes sense. Since the TableModel isn't actively used yet, this approach maintains good code organization while planning for future improvements.
LGTM 👍 🚢 |
Agreed! These changes look good to merge. The architecture improvements are solid, and the follow-up issues will handle the minor adjustments we discussed. Nice work on this PR! |
Details
bubbletea
: syncs unit tests.bubbletea
: syncs ChoicesModel.Update method.bubbletea
: adds Model updates and improves Bubbletea.Update method.bubbletea
: syncs unit tests.bubbletea
: removes unused ChoicesModel.table field.bubbletea
: handles current model update method upstream.bubbletea/tablemodel
: adds bubbletea table.Test Plan
CLI
works as expected via./bin/dev.sh
:CLI
works as expected via./bin/test.sh
: