Print warning when get_package_share_directory() does not exist (Fix #74)#77
Print warning when get_package_share_directory() does not exist (Fix #74)#77audrow merged 2 commits intoament:masterfrom
Conversation
audrow
left a comment
There was a problem hiding this comment.
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.
Is there a better way to remove the duplication between this and what's below?
There was a problem hiding this comment.
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.
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. |
audrow
left a comment
There was a problem hiding this comment.
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