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

Some simple proposed renames #751

Merged
merged 4 commits into from
Dec 8, 2019
Merged

Conversation

philip-peterson
Copy link
Contributor

In the process of trying to grok the definitions of types in agent.rs, a few renames seemed as though they may assist with reading the existing literature. Some of these I feel more strongly about than others, but it would be interesting to hear thoughts on these renames. Worth noting as well that I do not necessarily have the full context here, so these changes are offered in the spirit of sparking discussion.

Some of these would result in publicly backwards-incompatible changes, though pre-1.0 would probably be the best time to implement them, as such. The renames are as following:

  • AgentUpdate -> AgentUpdateEvent
  • Agent::handle -> Agent::handle_msg_from_worker
  • AgentEnvelope -> AgentCarrier - This one was confusing because arguably an Envelope could be something carried by an agent, as it evokes imagery similar to "packet." With this wording, it's more clear that the agent is being carried by the AgentCarrier.
  • Responder::response -> Responder::send_response - Since this function has no return value, it seems to be necessary for it to trigger a side effect. As such, I would propose a more verb-like name.

Any thoughts would be appreciated!

@jstarry
Copy link
Member

jstarry commented Nov 25, 2019

Thanks @philip-peterson, agreed that public api renames are better to do now rather than post 1.0.

  • I don't feel too strongly about AgentUpdate vs AgentUpdateEvent. Maybe AgentEvent would be better?
  • Agent::handle -> Agent::handle_msg_from_worker doesn't look right. Agent::handle could be handling input messages from other agents or the app itself. I think it's fine as is. Maybe Agent::handle_input would be better.
  • AgentEnvelope -> AgentCarrier. I actually prefer envelope. It carries an agent reference as well as an AgentUpdate. Or more explicitly an AgentUpdateRunnable.
  • Agreed that Responder::response should be a verb. I would actually go with Responder::respond instead though.

Thoughts? tag @hgzimmerman

@hgzimmerman
Copy link
Member

hgzimmerman commented Nov 25, 2019

  • I don't feel strongly about it either, but AgentUpdate sort of implies that the agent exists, where one of the variants is responsible for signaling the creation of an agent. I think AgentEvent or maybe even more unambiguously an InternalAgentEvent or AgentLifecycleEvent might be better.
  • I like handle_input, but don't feel too strongly.
  • Agreed on AgentEnvelope, I don't think it needs a change.
  • Agreed on Responder::respond

@philip-peterson
Copy link
Contributor Author

Fair enough points.

  • I agree with InternalAgentEvent or AgentLifecycleEvent
  • Responder::respond seems good too
  • AgentEnvelope still seems rather strange, I would picture it as the envelope formed by an Agent. But don't feel too strongly about this.
  • Agent::handle_input sounds like a reasonable alternative to my proposal.

Thanks for the feedback!

@mdtusz
Copy link
Contributor

mdtusz commented Nov 28, 2019

I'd agree with keeping Agent::handle - it mentally maps well to what is used in the actix handler trait and to me at least, it's fairly clear what it does.

@jstarry
Copy link
Member

jstarry commented Dec 7, 2019

@philip-peterson what's your plan for this PR?

@philip-peterson
Copy link
Contributor Author

Planning to rebase and just apply the changes that were approved, just haven't gotten to it yet, will follow up soon though.

@jstarry
Copy link
Member

jstarry commented Dec 8, 2019

I'd agree with keeping Agent::handle - it mentally maps well to what is used in the actix handler trait and to me at least, it's fairly clear what it does.

This is a good point, but we have already deviated from actix by calling actors agents

@jstarry jstarry merged commit bf06e36 into yewstack:master Dec 8, 2019
llebout pushed a commit to llebout/yew that referenced this pull request Jan 20, 2020
* AgentUpdate -> AgentLifecycleEvent

* Responder::response -> Responder::respond

* AgentLink::response -> AgentLink::respond

* Agent::handle -> Agent::handle_input
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants