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.

Sign up for GitHub

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

Extend package discovery to support passive mechanisms #685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Extend package discovery to support passive mechanisms #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions colcon_core/package_discovery/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
"""

"""The version of the discovery extension interface."""
EXTENSION_POINT_VERSION = '1.0'
EXTENSION_POINT_VERSION = '1.1'

"""The default priority of discovery extensions."""
PRIORITY = 100

def has_default(self):
"""
Check if the extension has a default parameter is none are provided.
Check if the extension has a default parameter if none are provided.

The method is intended to be overridden in a subclass.

Expand Down Expand Up @@ -62,7 +62,9 @@
This method must be overridden in a subclass.

:param args: The parsed command line arguments
:returns: True if `discover()` should be called, False otherwise
:returns: True if `discover()` should be called, False if no
parameters were given, or None if this extension has no
parameters to be specified.
:rtype: bool
"""
raise NotImplementedError()
Expand Down Expand Up @@ -219,6 +221,7 @@
def _get_extensions_with_parameters(
args, discovery_extensions
):
explicitly_specified = False
with_parameters = OrderedDict()
for extension in discovery_extensions.values():
logger.log(
Expand All @@ -234,9 +237,13 @@
f"'{extension.PACKAGE_DISCOVERY_NAME}': {e}\n{exc}")
# skip failing extension, continue with next one
else:
if has_parameter:
with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension
return with_parameters
if has_parameter is not None:
Copy link
Contributor

@j-rivero j-rivero Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This nested conditions are hard to read for a newbie in the code like me.

We could go explicit on has_parameter accepted values:

  diff --git a/colcon_core/package_discovery/__init__.py b/colcon_core/package_discovery/__init__.py                                                                                          
  index 165d7d3..44a897c 100644                                                                                                                                                               
  --- a/colcon_core/package_discovery/__init__.py                                                                                                                                             
  +++ b/colcon_core/package_discovery/__init__.py                                                                                                                                             
  @@ -236,13 +236,20 @@ def _get_extensions_with_parameters(                                                                                                                                  
                   'Exception in package discovery extension '                                                                                                                                
                   f"'{extension.PACKAGE_DISCOVERY_NAME}': {e}\n{exc}")                                                                                                                       
               # skip failing extension, continue with next one                                                                                                                               
  +            continue                                                                                                                                                                       
  +                                                                                                                                                                                           
  +        if has_parameter is True:                                                                                                                                                          
  +            explicitly_specified = True                                                                                                                                                    
  +        if has_parameter is False:                                                                                                                                                         
  +            continue                                                                                                                                                                       
  +        elif has_parameter is None:                                                                                                                                                        
  +            pass                                                                                                                                                                           
           else:                                                                                                                                                                              
  -            if has_parameter is not None:                                                                                                                                                  
  -                if has_parameter:                                                                                                                                                          
  -                    explicitly_specified = True                                                                                                                                            
  -                else:                                                                                                                                                                      
  -                    continue                                                                                                                                                               
  -            with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension                                                                                                                  
  +            assert 'has_parameter returned a value different of True, '\                                                                                                                   
  +                   'False or None'                                                                                                                                                         
  +                                                                                                                                                                                           
  +        with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension                                                                                                                      
  +                                                                                                                                                                                           
       return with_parameters if explicitly_specified else OrderedDict()

or in a more packed form:

  --- a/colcon_core/package_discovery/__init__.py                                                                                                                                             
  +++ b/colcon_core/package_discovery/__init__.py                                                                                                                                             
  @@ -236,13 +236,15 @@ def _get_extensions_with_parameters(                                                                                                                                  
                   'Exception in package discovery extension '                                                                                                                                
                   f"'{extension.PACKAGE_DISCOVERY_NAME}': {e}\n{exc}")                                                                                                                       
               # skip failing extension, continue with next one                                                                                                                               
  -        else:                                                                                                                                                                              
  -            if has_parameter is not None:                                                                                                                                                  
  -                if has_parameter:                                                                                                                                                          
  -                    explicitly_specified = True                                                                                                                                            
  -                else:                                                                                                                                                                      
  -                    continue                                                                                                                                                               
  -            with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension                                                                                                                  
  +            continue                                                                                                                                                                       
  +                                                                                                                                                                                           
  +        if has_parameter is False:                                                                                                                                                         
  +            continue                                                                                                                                                                       
  +        elif has_parameter:                                                                                                                                                                
  +            explicitly_specified = True                                                                                                                                                    
  +                                                                                                                                                                                           
  +        with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension                                                                                                                      
  +                                                                                                                                                                                           
       return with_parameters if explicitly_specified else OrderedDict()

if has_parameter:
explicitly_specified = True
else:
continue

Check warning on line 244 in colcon_core/package_discovery/__init__.py

View check run for this annotation

Codecov / codecov/patch

colcon_core/package_discovery/__init__.py#L244 

Added line #L244 was not covered by tests
with_parameters[extension.PACKAGE_DISCOVERY_NAME] = extension
return with_parameters if explicitly_specified else OrderedDict()


def _discover_packages(
Expand Down
5 changes: 4 additions & 1 deletion test/test_package_discovery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def test_discover_packages():
return_value={PackageDescriptor('/extension1/pkg1')})
extensions['extension2'].discover = Mock(
return_value={PackageDescriptor('/extension2/pkg1')})
extensions['extension2'].has_parameters = Mock(return_value=None)

descs = discover_packages(None, None, discovery_extensions=extensions)
assert len(descs) == 2
Expand All @@ -126,11 +127,13 @@ def test_discover_packages():
PackageDescriptor('/extension3/pkg2')})

descs = discover_packages(None, None, discovery_extensions=extensions)
assert len(descs) == 2
assert len(descs) == 3
expected_path = '/extension3/pkg1'.replace('/', os.sep)
assert expected_path in (str(d.path) for d in descs)
expected_path = '/extension3/pkg2'.replace('/', os.sep)
assert expected_path in (str(d.path) for d in descs)
expected_path = '/extension2/pkg1'.replace('/', os.sep)
assert expected_path in (str(d.path) for d in descs)


def test_expand_dir_wildcards():
Expand Down
Loading