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

Update action goal state diagram #226

Merged
merged 2 commits into from
Apr 16, 2019
Merged

Conversation

jacobperron
Copy link
Member

  • Change transition labels to match text in design doc.
  • Renamed cancel_goal transition to request_cancel.
    This makes it more clear from the name that it is a request to cancel and not the actual canceling of a goal.

Related to changes happening for ros2/rcl#399.

* Change transition labels to match text in design doc.
* Renamed cancel_goal transition to request_cancel.
  This makes it more clear from the name that it is a *request* to cancel and not the actual canceling of a goal.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron self-assigned this Mar 30, 2019
@jacobperron jacobperron added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 30, 2019
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM! Bummer we can't keep cancel_goal, it was more consistent with send_goal.

articles/actions.md Outdated Show resolved Hide resolved
@jacobperron
Copy link
Member Author

Bummer we can't keep cancel_goal, it was more consistent with send_goal

We could keep it, although, IMO, "request_cancel" is more clear.

@dirk-thomas
Copy link
Member

I am not sure the "request" in the name is better. With any service your request is just that - a request - and the response could be "no, not going to do it". Therefore I think cancel_goal was the better name and doesn't need to be changed.

@jacobperron
Copy link
Member Author

I am not sure the "request" in the name is better. With any service your request is just that - a request - and the response could be "no, not going to do it". Therefore I think cancel_goal was the better name and doesn't need to be changed.

We should distinguish the cancel request event from the event where the goal is actually canceled (see ros2/rcl#399 (comment)). I think the events "cancel_goal" and "cancel" are more ambiguous than "request_cancel" and "cancel". In any case, it's probably not helping that we're conflating the user request to cancel and the goal state transition.

@dirk-thomas
Copy link
Member

We should distinguish the cancel request event from the event where the goal is actually canceled (see ros2/rcl#399 (comment)). I think the events "cancel_goal" and "cancel" are more ambiguous than "request_cancel" and "cancel". In any case, it's probably not helping that we're conflating the user request to cancel and the goal state transition.

From the wording "cancel" alone it is again not clear if that is just a request, it is about to happen or it is already done.

I would argue that the natural language already has a clean way to distinguish these two cases:

  • cancel_goal: the verb expresses what you want to do with the subject - to cancel a specific goal
  • goal_cancelled or just cancelled describes that has happens to a specific goal - that it has been cancelled

request_cancel -> cancel_goal
cancel -> canceled

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

I've reverted the change to the "cancel_goal" transition and renamed the "cancel" transition to "canceled" as per @dirk-thomas's suggestion. I've updated the state diagram accordingly.

@lbegani
Copy link

lbegani commented Apr 12, 2019

I've reverted the change to the "cancel_goal" transition and renamed the "cancel" transition to "canceled" as per @dirk-thomas's suggestion. I've updated the state diagram accordingly.

Typo: Should it not be cancelled instead of canceled

@clalancette
Copy link
Contributor

Typo: Should it not be cancelled instead of canceled

It depends on which flavor of English you learned: https://www.grammarly.com/blog/canceled-vs-cancelled/

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM too!

@hidmic
Copy link
Contributor

hidmic commented Apr 15, 2019

Though if I get really picky, I find the use of canceled, an adjective, along with abort or succeed, both verbs, a bit inconsistent. But I can't think of a better naming right now.

@dirk-thomas
Copy link
Member

I find the use of canceled, an adjective, along with abort or succeed, both verbs, a bit inconsistent.

Could be changed to succeeded / aborted?

@hidmic
Copy link
Contributor

hidmic commented Apr 15, 2019

Could be changed to succeeded / aborted?

I'm somewhat inclined towards verbs, but yeah, that's the alternative. Anyways, it isn't super important.

@dirk-thomas
Copy link
Member

I'm somewhat inclined towards verbs

Using verbs to describe a status seems off to me.

@hidmic
Copy link
Contributor

hidmic commented Apr 15, 2019

Using verbs to describe a status seems off to me.

Agreed, but IIUC it's not the status or state but the event that we're describing here.

@jacobperron
Copy link
Member Author

jacobperron commented Apr 15, 2019

Agreed, but IIUC it's not the status or state but the event that we're describing here.

Correct. These are the names of the transitions between states. I believe the word "canceled" can be used as an adjective and as a verb.

@jacobperron jacobperron merged commit 951f732 into gh-pages Apr 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the jacob/rename_action_events branch April 16, 2019 11:45
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants