-
Notifications
You must be signed in to change notification settings - Fork 27
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
Print warning when get_package_share_directory() does not exist (Fix #74) #77
Conversation
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.
Looks pretty good. Thanks for the PR. I have a comment on removing duplication.
Is there a way you could add a test for this?
Also, could you change the PR's title to be something more descriptive?
the_path = os.path.join(get_package_prefix(package_name), 'share', package_name) | ||
if print_warning and not os.path.exists(the_path): | ||
warnings.warn(f'Share directory for {package_name} ({the_path}) does not exist.', stacklevel=2) | ||
return the_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.
Is there a better way to remove the duplication between this and what's below?
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.
So I don't like the duplication either, but I did have a specific purpose with it. Now, I'm new to the warnings
module, so maybe there's a better way to do it.
Let's say I have my_awesome_script.py
from ament_index_python import get_package_share_directory, get_package_share_path
print(get_package_share_directory('trogdor'))
print(get_package_share_path('trogdor'))
If I run it with the code as-is, I get the following output
! ament_index_python/ > python3 my_awesome_script.py
my_awesome_script.py:3: UserWarning: Share directory for trogdor (/home/dlu/colcon_ws/install/trogdor/share/trogdor) does not exist.
print(get_package_share_directory('trogdor'))
/home/dlu/colcon_ws/install/trogdor/share/trogdor
my_awesome_script.py:4: UserWarning: Share path for trogdor (/home/dlu/colcon_ws/install/trogdor/share/trogdor) does not exist.
print(get_package_share_path('trogdor'))
/home/dlu/colcon_ws/install/trogdor/share/trogdor
Note that it reports the warning on lines 3 and 4 of my_awesome_script.py
Now, you can remove the duplication and just have get_package_share_directory
report the warning, but then you get
! ament_index_python/ > python3 my_awesome_script.py
my_awesome_script.py:3: UserWarning: Share directory for trogdor (/home/dlu/colcon_ws/install/trogdor/share/trogdor) does not exist.
print(get_package_share_directory('trogdor'))
/home/dlu/colcon_ws/install/trogdor/share/trogdor
/home/dlu/..../ament_index/ament_index_python/ament_index_python/packages.py:100: UserWarning: Share directory for trogdor (/home/dlu/colcon_ws/install/trogdor/share/trogdor) does not exist.
return pathlib.Path(get_package_share_directory(package_name))
/home/dlu/colcon_ws/install/trogdor/share/trogdor
Now the warning is being reported on line 100 of packages.py
which is less helpful. Theoretically you could add a different parameter to ensure that the proper stacklevel
was set, but I thought the API was cleaner to add print_warning
rather than _stacklevel_increment
or something.
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 agree that is more helpful. Thanks for explaining it.
Signed-off-by: David V. Lu <davidvlu@gmail.com>
Also, I did add additional lines to |
No, it's fine - I think that I missed them on my first review. |
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.
Looks good to me. I'll run CI.
I'm going to go ahead and merge because the test that failed is a known failure and is unrelated to this PR (private repo, sorry): |
Thanks for the PR, @DLu! |
FYI: buildfarmer is a private repo. |
Fix for #74. With new tests, and added files to ensure no warnings on old tests.
Signed-off-by: David V. Lu davidvlu@gmail.com