Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(robot-server): Add
GET
runs/:runId/currentState
#16402feat(robot-server): Add
GET
runs/:runId/currentState
#16402Changes from 7 commits
3defe68
b3705fb
2b0a4a7
378ed95
8d94ba0
3036941
039dd4f
377299e
ab064fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on grounding this data at a point in time
As implemented, this looks like it reports the "current" command with
GET /runs/{id}/commands
's definition of "current," which is:The "or" clause there unfortunately means that this link, on its own, is not sufficient to identify a point in time. The link will be the same between: (1) the run executing a command right now, and (2) the run having just executed a command and not yet having moved on to the next one. This will make it look to the client like time has not moved on and robot state has not changed, but in fact it has.
To solve this, we could either:
status
.lastCompletedCommand
. I don't think any other endpoints do this yet; it would be new.I'm leaning towards
lastCompletedCommand
if that info happens to already be easy to access. It seems harder for a client to use wrongly.Some thoughts on whether to call this "active", "current", or something else else:
Pending all the stuff above, assuming we continue to use the same "current command" semantics that
GET /runs/{id}/commands
does right now, we should either:current
to matchGET /runs/{id}/commands
currentlyRunningOrMostRecentlyRun
to spell out what it actually does, and, separately, phase outcurrent
inGET /runs/{id}/commands
and replace it withcurrentlyRunningOrMostRecentlyRun
there too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think
current
to matchGET /runs/{id}/commands
is the right way. The only thing worse than something being subtlely wrong is something being inconsistent and only sometimes subtlely wrongThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in-person with Jamey: We'll probably go with
lastCompletedCommand
to avoid concurrency gotchas.