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

made "batchMsg" public since it should be #114

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

treilik
Copy link
Contributor

@treilik treilik commented Jul 8, 2021

because it carry's no special meaning which should be private,
and there are use cases where its necessary to access the Batched
Messages. (I know because i have one ;)

@meowgorithm
Copy link
Member

Hey, Treilik! What’s your use case? Asking because quitMsg is similarly not exposed and I haven’t encountered a reason to expose either that nor batchMsg yet.

@treilik
Copy link
Contributor Author

treilik commented Jul 8, 2021

Hi meowgorithm!
in my boxer-project ich have the case that i compose a recursive tree of tea.model's which call each subsequently the Update-method of the there children.
Because they can have more children, the returned cmd's are Batched and returned.
Since i use the cmd's as events (yet), i want to be able to check the type and value of the returned (batched) cmd's.
For me, there is a way around this.
But since i thought that it would be anyway more idiomatic,
to have a public method/function return something public, so i opened this pr.

@muesli muesli changed the title made "batchMsg" public since it should be, made "batchMsg" public since it should be Jul 8, 2021
@muesli
Copy link
Contributor

muesli commented Jul 8, 2021

I think this is a reasonable request, as a BatchMsg is merely a type alias for a slice of Cmds, and once you Batch them up, you have no other way of accessing them.

I guess the danger is, that people could start using it in ways that make it a bit of a misnomer: when they actually just want a slice of Cmds, not necessarily in the form of a message. Just to make sure we understand the request correctly, could you point us to a place in boxer where you would make use of that newly introduced public type?

@treilik
Copy link
Contributor Author

treilik commented Jul 9, 2021

No, not yet, but i can explain the case:
So i want to provide a way to let the models of the different boxes communicate with each other.
Since it is a recursive architecture the boxes do not know about each other.
So at compile time they have a address, and other boxes can an send a cmd which msg will be routed to the box having the address.
Since the content of the sending box can return a batchMsg, which will be embedded into the message send to the address, the receiving box receives a (private) batchMsg and can't acces the content.
content (c1) from leave box (l1) returns a batchMsg:

bubbletea runtime
        |
        n
       / \
     l1   l2
  ↑  /     \
 bM c1      c2

leave box (l1) embeddes returned cmd into a addressCmd

bubbletea runtime
   ↑    |
   ↑    n
   ↑   / \
  aC l1   l2
     /     \
    c1      c2

bubbletea calls the addressCmd and returns the addressMsg in which the batchMsg recides
and calls the update of the rootNode (n) with it.

bubbletea runtime   aM
        |            ↓
        n
       / \
     l1   l2
     /     \
    c1      c2

the rootNode (n) decides to which child it should send the addressMsg based on the address.
And sends it to (l2)

bubbletea runtime
        |
        n           aM
       / \           ↓
     l1   l2
     /     \
    c1      c2

l2 recieves the addressMsg and since it is addressed to it, it takes the payload (a batchMsg after calling the cmd) and tries to act upon it.
But since its private it can't.

bubbletea runtime
        |
        n
       / \
     l1   l2     bM
     /     \
    c1      c2

As i sad i have a way around it, and i know that since i bypass a cmd around the bubbletea runtime, i currently misuse the cmd and bubbletea but that is/was my use-case.

@meowgorithm
Copy link
Member

Thanks for the very detailed description, Treilik. One thing I'm not following is how you’re accessing batchMsgs.

batchMsg is produced by BatchCmd when it is called in the runtime (here), however it's consumed a few lines down in the main loop (here) before it ever reaches a Bubble Tea program’s main Update.

Let me know: I may be missing something.

@treilik
Copy link
Contributor Author

treilik commented Jul 12, 2021

I think this is the crucial part you are missing:

Since the content of the sending box can return a batchMsg, which will be embedded into the message send to the address, the receiving box receives a (private) batchMsg and can't acces the content.

or to put it into other words:
The Update is not called (directly) from the Bubble Tea runtime, but from one of my Update from boxer. (That's way i drew the tree diagrams)
and thus i can embed the returned cmd into a new message which will bypass the bubbletea runtime and thus i have the problem of the private batchMsg.

This is my use-case, but maybe the core problem is, that as soon as a other program (my boxer) calls by it self a Update function of a 'tea.Model', and wants to act upon the 'cmd' this returns, a private return value is not helpful.

Hope this helps to clear up my problem, but aside from this, i do think that it would be nicer if public function return public types.

Thanks for taking the time :)
Cheers!

@bashbunni bashbunni added the question Further information is requested label Apr 22, 2022
@muesli muesli closed this Jun 20, 2022
@muesli muesli reopened this Jun 20, 2022
@muesli muesli requested a review from meowgorithm June 20, 2022 13:23
Copy link
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

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

Alright, this makes sense. Let’s do it. Thanks for the contribution, @treilik!

There's no good reason to keep it private. Exporting it helps
testability, debugging, and allows for a few special model.Update
implementations.
@muesli muesli closed this Oct 22, 2022
@muesli muesli reopened this Oct 22, 2022
@muesli muesli added enhancement New feature or request and removed question Further information is requested labels Oct 22, 2022
@muesli muesli merged commit 918d357 into charmbracelet:master Oct 22, 2022
@muesli
Copy link
Contributor

muesli commented Oct 22, 2022

It's been a long time in the making! Thank you @treilik!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants