-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor SDF handling logic for non-existing string paths #50
Conversation
The |
332859c
to
73420ab
Compare
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'm not 100% sure that the new logic covers entirely the old logic. Also, the previous refactoring might have introduced some unchecked cases.
It's probably worth to have a unit test for such a core component of the library like loading the supported types of resources.
I added additional tests and refactored forcing |
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 the three original cases were correct, and now something is missing:
- If the resource is a path, expect that is exists and read its content.
- If the resource is a string:
- a) if its length is shorter than the max path length, check if it's a file. If it is, read its content, otherwise treat it as a string containing the URDF/SDF.
- b) if its length is longer than the max path length, treat is as a string containing the URDF/SDF.
How is 2a handled now? The logic seems changed.
c4c3bc8
to
e0cf6ad
Compare
Solved by rebase. I had to make the assumption that the passed string path has an extension, I couldn't find any other way to distinguish between a URDF shorter than MAX_PATH and a string path. |
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.
Great! Now it looks much better, thanks @flferretti.
The non-existing paths will be handled by
pathlib.Path.read_text()
, eventually raising aFileNotFoundError
.