Skip to content

Update action goal state diagram#226

Merged
jacobperron merged 2 commits intogh-pagesfrom
jacob/rename_action_events
Apr 16, 2019
Merged

Update action goal state diagram#226
jacobperron merged 2 commits intogh-pagesfrom
jacob/rename_action_events

Conversation

@jacobperron
Copy link
Copy Markdown
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 <jacob@openrobotics.org>
@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
Copy Markdown

@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.

@jacobperron
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <jacob@openrobotics.org>
@jacobperron
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

I'm somewhat inclined towards verbs

Using verbs to describe a status seems off to me.

@hidmic
Copy link
Copy Markdown

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
Copy Markdown
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