SDFormat 1.7 parsing stages: add links to libsdformat9 code #28
Conversation
Add links in the parsing stages of the SDFormat 1.7 proposal to the code that implements those parsing stages in libsdformat9. Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey and @scpeters)
a discussion (no related file):
BTW At the high-level, re-reading this and seeing the heavy duplication between //model and //world, it makes me wish we had a better abstraction to collapse some of this, and thus possibly simplify these docs.
However, I don't have a good concrete suggestion atm...
pose_frame_semantics/proposal.md, line 1385 at r1 (raw file):
There are *eight* phases for validating the kinematics data in a model. In libsdformat, the `sdf::readFile` and `sdf::readString` API's perform parsing
nit "API's" => "APIs"?
pose_frame_semantics/proposal.md, line 1386 at r1 (raw file):
There are *eight* phases for validating the kinematics data in a model. In libsdformat, the `sdf::readFile` and `sdf::readString` API's perform parsing stage 1, `ign sdf --check` performs all parsing stages,
nit Can you capitalize "Stage" for "Stage 1"?
pose_frame_semantics/proposal.md, line 1386 at r1 (raw file):
There are *eight* phases for validating the kinematics data in a model. In libsdformat, the `sdf::readFile` and `sdf::readString` API's perform parsing stage 1, `ign sdf --check` performs all parsing stages,
Seeing ign sdf --check is a bit confusing... Can you try to clarify that some, and possibly reorder how it's stated?
e.g.
There are eight... and different parts of the `libsdformat` handle differing sets of stages:
- `sdf::readFile` and `sdf:::readString` APIs perform basic parsing of Stage 1
- `sdf::Root::Load` performs most parsing stages, but skips some of the more expensive checks.
- `ign sdf --check` performs all parsing stages, including more expensive checks
pose_frame_semantics/proposal.md, line 1402 at r1 (raw file):
*that they are not reserved (*`__.*__` *or* `world`*)* and that sibling elements of *any* type have unique names. This includes but is not limited to models, actors, links, joints,
nit Missing comma before "but"?
pose_frame_semantics/proposal.md, line 1406 at r1 (raw file):
This step is distinct from validation with the schema because the schema only confirms the existence of name attributes, not their content. *In `libsdformat9`, names are checked for empty strings by the `sdf::readFile` and*
It's kinda hard to read this chunk as a single paragraph.
Can this have separation with line breaks (<br/>) or bullet points?
pose_frame_semantics/proposal.md, line 1407 at r1 (raw file):
only confirms the existence of name attributes, not their content. *In `libsdformat9`, names are checked for empty strings by the `sdf::readFile` and* *`sdf::readString` API's (via [Param::SetFromString](https://github.com/osrf/sdformat/blob/sdformat9_9.2.0/src/Param.cc#L452-L457)),*
Can you remove the italics here?
Or rather, is there a reason you want them?
FTR, GitHub has some pretty awesome Rich Text Diff:
https://github.com/osrf/sdf_tutorials/pull/28/files#diff-c6158d11e18b960595aa890be6b52a0a
pose_frame_semantics/proposal.md, line 1515 at r1 (raw file):
7. ***Check `//pose/@relative_to` attribute values:*** For each `//pose` that is not `//model/pose` (ie. `//link/pose`,
nit I think "ie." should be "e.g."?
* reorder code paths used in parsing and add bullets, per suggestion * API's -> APIs * ie. -> i.e. * replace an ie. with e.g.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW At the high-level, re-reading this and seeing the heavy duplication between
//modeland//world, it makes me wish we had a better abstraction to collapse some of this, and thus possibly simplify these docs.However, I don't have a good concrete suggestion atm...
it's true, we could potentially merge these since they are mostly similar, but I would save that for later
pose_frame_semantics/proposal.md, line 1385 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit "API's" => "APIs"?
Done
pose_frame_semantics/proposal.md, line 1386 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Seeing
ign sdf --checkis a bit confusing... Can you try to clarify that some, and possibly reorder how it's stated?e.g.
There are eight... and different parts of the `libsdformat` handle differing sets of stages: - `sdf::readFile` and `sdf:::readString` APIs perform basic parsing of Stage 1 - `sdf::Root::Load` performs most parsing stages, but skips some of the more expensive checks. - `ign sdf --check` performs all parsing stages, including more expensive checks
how does it look in de03efa?
pose_frame_semantics/proposal.md, line 1386 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you capitalize "Stage" for "Stage 1"?
Done
pose_frame_semantics/proposal.md, line 1402 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Missing comma before "but"?
I think a comma is not needed because this is a compound predicate (multiple verbs) with only one subject
pose_frame_semantics/proposal.md, line 1407 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can you remove the italics here?
Or rather, is there a reason you want them?
FTR, GitHub has some pretty awesome Rich Text Diff:
https://github.com/osrf/sdf_tutorials/pull/28/files#diff-c6158d11e18b960595aa890be6b52a0a
it's not shown in the diff of this PR, but the introduction to the parsing stages explains that italics are used to indicate that something has changed between this proposal and the behavior documented in sdf 1.4
pose_frame_semantics/proposal.md, line 1515 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit I think "ie." should be "e.g."?
Done
Hopefully this makes it easier to read. Signed-off-by: Steve Peters <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
pose_frame_semantics/proposal.md, line 1406 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
It's kinda hard to read this chunk as a single paragraph.
Can this have separation with line breaks (
<br/>) or bullet points?
ok, I made an attempt in 0a8e9c6, let me know what you think
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI)
pose_frame_semantics/proposal.md, line 1406 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
ok, I made an attempt in 0a8e9c6, let me know what you think
These are now showing up in a code block.
Signed-off-by: Steve Peters <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI)
pose_frame_semantics/proposal.md, line 1406 at r1 (raw file):
Previously, azeey (Addisu Taddese) wrote…
These are now showing up in a code block.
I think 108130d improves it
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI)
a discussion (no related file):
just checking on when it's safe to merge a reviewable PR; @EricCousineau-TRI you gave a LGTM but there are unresolved conversations. is it customary to wait for them all to be resolved?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)
a discussion (no related file):
Previously, scpeters (Steve Peters) wrote…
just checking on when it's safe to merge a reviewable PR; @EricCousineau-TRI you gave a LGTM but there are unresolved conversations. is it customary to wait for them all to be resolved?
Yup, it is customary to wait. Resolved 'em now!
pose_frame_semantics/proposal.md, line 1386 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
how does it look in de03efa?
OK Looks great!
pose_frame_semantics/proposal.md, line 1402 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
I think a comma is not needed because this is a compound predicate (multiple verbs) with only one subject
OK SGTM.
pose_frame_semantics/proposal.md, line 1407 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
it's not shown in the diff of this PR, but the introduction to the parsing stages explains that italics are used to indicate that something has changed between this proposal and the behavior documented in sdf 1.4
OK Ah, thanks for explaining!
There was a problem hiding this comment.
Dismissed @scpeters from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved
Add links in the parsing stages of the SDFormat 1.7 proposal to the code that implements those parsing stages in libsdformat9.
Preview: http://sdformat.org/tutorials?tut=pose_frame_semantics_proposal&cat=pose_semantics_docs&branch=proposal_stages_code_links#1-model
This change is