clalancette commented 4 years ago

Importing pkg_resources is slow; really slow. pkg_resources is also somewhat deprecated. For both of these reaons, we would prefer to move away from pkg_resources and start using importlib instead.

sros2 was using 'resource_filename' from pkg_resources. The equivalent in importlib is importlib.resources, but there is a catch. As of Python 3.8, importlib.resources does not support getting data from subdirectories. The workaround is to make each level of the directory structure a python module, so that the module name can be looked up. Python 3.9 will have support for directories for resources, but that won't help on Ubuntu Focal. More information is in https://gitlab.com/python-devs/importlib_resources/issues/58

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

See ros2/ros2#919 for a lot more details on why we want to do this.

clalancette commented 4 years ago

Could you please add a comment or TODO to the added __init__.py files to explain why they're needed and in what conditions they can be safely removed?

Good call. I added the comment to all of the init files in b3a53fd.

codecov[bot] commented 4 years ago

Codecov Report

Merging #225 into master will increase coverage by 0.36%. The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   76.48%   76.84%   +0.36%     
==========================================
  Files          17       23       +6     
  Lines         591      609      +18     
  Branches       53       53              
==========================================
+ Hits          452      468      +16     
- Misses        119      121       +2     
  Partials       20       20

Flag	Coverage Δ
#unittests	`76.84% <92.00%> (+0.36%)`	:arrow_up:

Impacted Files	Coverage Δ
sros2/sros2/policy/__init__.py	`84.31% <89.47%> (-0.31%)`	:arrow_down:
sros2/sros2/policy/defaults/__init__.py	`100.00% <100.00%> (ø)`
sros2/sros2/policy/defaults/dds/__init__.py	`100.00% <100.00%> (ø)`
sros2/sros2/policy/schemas/__init__.py	`100.00% <100.00%> (ø)`
sros2/sros2/policy/schemas/dds/__init__.py	`100.00% <100.00%> (ø)`
sros2/sros2/policy/templates/__init__.py	`100.00% <100.00%> (ø)`
sros2/sros2/policy/templates/dds/__init__.py	`100.00% <100.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cd234c1...b3a53fd. Read the comment docs.

mikaelarguedas commented 4 years ago

CI failure unrelated and due to https://github.com/ros2/sros2/issues/221.

@clalancette feel free to merge this along the other PRs

clalancette commented 4 years ago

CI failure unrelated and due to #221.

@clalancette feel free to merge this along the other PRs

Sounds good, thanks!

clalancette commented 4 years ago

All right, CI changes are merged as part of https://github.com/ros2/ci/pull/482 . CI is green in https://github.com/ros2/ros2/issues/919 , and I have 3 approvals. Thanks for the reviews, merging.

ros2 / sros2

Remove the use of pkg_resources. #225

Codecov Report