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

Automatically detect sdf extension from parsed string #42

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

flferretti
Copy link
Contributor

This PR enable detecting the XML type from the parsed string.

Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

Thanks @flferretti for this contribution! This PR introduces the automatic detection of either the URDF or SDF type when the model description is a string containing the content. Before this PR, in this case the user had to pass a is_urdf argument.

If the model is a pathlib.Path, we read its extension for the file name, and it cannot be wrong. Having some heuristics on the URDF detection also when the passed description is a string, is definitely useful. However, the proposed solution is very fragile, and I suggested an more robust solution.

Furthermore, I'd not change the APIs by removing entirely the is_urdf argument. With this PR, this argument will be useless in most cases, now extended to models passed as strings. Though, there might be edge cases where forcing either SDF or URDF is still necessary, and I'd suggest to leave this is_urdf argument there to address them. We can update the docstring to something like: Force the SDF resource to be treated as URDF if the automatic detection fails.

src/rod/sdf/sdf.py Outdated Show resolved Hide resolved
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @flferretti for your contribution.

@diegoferigo diegoferigo merged commit 0cc5039 into ami-iit:main Sep 23, 2024
16 checks passed
@flferretti flferretti deleted the feature/auto_sdf_detection branch September 23, 2024 07:40
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.

2 participants