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

Schema accessor #1393

Open
wants to merge 3 commits into
base: sdf14
Choose a base branch
from
Open

Conversation

aagrawal05
Copy link

@aagrawal05 aagrawal05 commented Apr 5, 2024

🎉 New feature

Closes #1376

Summary

  1. I've created a script format_sdf.py which reads from the /sdf/1.11 folder and generates/updates the source for all the relevant classes in the src/ and include/ directories as per the suggested implementation.
  2. There were DOM elements navsat.sdf and forcetorque.sdf for which the script does not work and I had to manually edit the script because their naming convention did not match the camel-case conversion (they would need to be nav_sat.sdf and force_torque.sdf respectively to function correctly with the script) This may be undesirable from a code style perspective but may be a breaking change—this is a minor point but I just wanted to point it out.
  3. The code builds successfully, I have not thoroughly tested it but it does seem to work.
  4. I can add tests for the SchemaFile in all the DOM element tests but I thought it might be overkill because the names are hardcoded in the method anyways. I can add it easily with the script if needed.

Test it

Tests can be done manually as mentioned by the OP in #1376 in his code example.
I could also add tests in the DOM element unit tests, but that might be overkill because the values are hard-coded.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Apr 5, 2024
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have a few comments that should be easy to address.

Regarding testing, I don't think we should create new tests for this, but one way we can make sure it's working is by using SchemaFile() instead of hard coded file names in ToElement functions. Perhaps you can update your script to make that change as well?

.DS_Store Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file

src/Actor.cc Outdated
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

src/Actor.cc Outdated
Comment on lines 824 to 828
const std::string& Actor::SchemaFile()
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to our style guide, this is sort of a last resort way to create variables with static storage. After reading through https://abseil.io/tips/140#string-view-mistake, I think the recommended way is:

Suggested change
const std::string& Actor::SchemaFile()
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
inline std::string_view Actor::SchemaFile()
{
static const char kSchemaFile[] = "actor.sdf";
return kSchemaFile;
}

@aagrawal05 aagrawal05 requested a review from azeey May 19, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Add schema file name accessor to each DOM type C++ class
2 participants