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.

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

Default provider is ignore #44

Merged
merged 5 commits into from
Feb 7, 2014
Merged

Default provider is ignore #44

merged 5 commits into from
Feb 7, 2014

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Feb 6, 2014

It's possible I'm misunderstanding how default providers work, but it appears that if there are multiple providers for an interface, and a default is defined, it isn't necessarily used. Instead, start_capability() just uses the first provider in the list:

https://github.com/osrf/capabilities/blob/master/src/capabilities/server.py#L620

@wjwwood
Copy link
Member

wjwwood commented Feb 4, 2014

I think you are correct, the default provider is respected when a capability starts another capability which it depends on, but not when the user passes any empty string as the preferred provider for the start_capability service call.

I'll fix this.

@wjwwood wjwwood added the bug label Feb 4, 2014
@wjwwood wjwwood self-assigned this Feb 4, 2014
@wjwwood
Copy link
Member

wjwwood commented Feb 6, 2014

I now have a test to reproduce this, fix coming soon.

@wjwwood
Copy link
Member

wjwwood commented Feb 6, 2014

Ok, fix pushed, I'll wait for CI to catch up and then merge.

@jonbinney for review

@@ -609,15 +609,14 @@ def __get_providers_for_interface(self, interface):
def __start_capability(self, capability, preferred_provider):
if capability not in self.__spec_index.interfaces.keys() + self.__spec_index.semantic_interfaces.keys():
raise RuntimeError("Capability '{0}' not found.".format(capability))
# If not preferred provider is given, use the default
Copy link
Contributor

Choose a reason for hiding this comment

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

If not preferred provider is given, use the default you must!

@jonbinney
Copy link
Contributor Author

testing it now on my setup

@jonbinney
Copy link
Contributor Author

Works for me, and i don't see any problems with the code. Thanks william!

@wjwwood
Copy link
Member

wjwwood commented Feb 6, 2014

I fixed the typo and cleaned up the code to have 100% coverage again. Waiting to merge until CI and coveralls catches up.

@jonbinney
Copy link
Contributor Author

But the typo was awesome :-(

@jonbinney
Copy link
Contributor Author

See - even travis fails without the typo...

@wjwwood
Copy link
Member

wjwwood commented Feb 7, 2014

😜

wjwwood added a commit that referenced this pull request Feb 7, 2014
Default provider is ignore
@wjwwood wjwwood merged commit babb952 into master Feb 7, 2014
@wjwwood wjwwood deleted the issue_44 branch February 7, 2014 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants