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
Changed progress Update method to return the specific Model in progress.go instead of the generic tea.Model #457
base: master
Are you sure you want to change the base?
Conversation
…ss.go instead of the generic tea.Model
Hi there! So the reason we didn't do this on type Model struct {
progress tea.Model
}
var cmd tea.Cmd
m.progress, cmd = m.progress.Update(msg)
if p, ok := m.progress.(progress.Model); ok {
fmt.Println(p.Percent())
} Would love to hear your reasoning for storing the struct as a |
I think the opposite is happening the existing code requires that assertion because the type it returns is tea.Model instead of the progress bar type. This removes the need for the assertion. |
I really think this is a matter of being a typo. If I look at all of the usages of update I see that most of them return the Example: Line 771 in ec88302
But progress bar instead returns a generic Line 206 in ec88302
By running grep I can see that most return $ grep -r "func.*Update(" *
README.md:func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
cursor/cursor.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
filepicker/filepicker.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
help/help.go:func (m Model) Update(_ tea.Msg) (Model, tea.Cmd) {
key/key.go:// func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
list/list.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
list/defaultitem.go:func (d DefaultDelegate) Update(msg tea.Msg, m *Model) tea.Cmd {
list/list_test.go:func (d itemDelegate) Update(msg tea.Msg, m *Model) tea.Cmd { return nil }
paginator/paginator.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
progress/progress.go:func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
spinner/spinner.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
stopwatch/stopwatch.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
table/table.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
textarea/textarea.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
textinput/textinput.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
timer/timer.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) {
viewport/viewport.go:func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) { The exceptions are only in the |
Ah, I misread the PR. Yep, the change probably makes sense. It wasn't a typo per se—but I agree that we should stick with the convention of the rest of Another option we've been discussing internally is a convention like the following: func (Model) Update(tea.Msg) (tea.Model, tea.Cmd)
func (*Model) UpdateInPlace(tea.Msg) tea.Cmd
// Or alternatively
func (Model) UpdateAsModel(tea.Msg) (Model, tea.Cmd) This would allow models to be composable, but also give users the option to skip unnecessary assertions. |
The only comment I'd have about the method with a pointer receiver is that some linters will complain if you have mix and match pointer receivers and regular ones. I don't know if it is valid criticism, and I'm not at my machine to double check, but I'm pretty sure Golang highlights them by default. |
@meowgorithm, how do you feel about merging this one? I agree that implementing the |
NOTE: I created this PR 1 month ago but apparently it was closed automatically when I deleted my fork
The progress bar code returns a generic tea.Model instead of the specific model in progress.go. This works but it means that you end up having to do a type assertion like this:
If it used the correct Model struct you could instead do this:
This PR changes the return type.