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

Add ignore_plugins parameter for get_status and get_hook_caller #25

Merged
merged 5 commits into from
Aug 27, 2018

Conversation

williamjamir
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #25 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #25   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         380    387    +7     
=====================================
+ Hits          380    387    +7

@williamjamir williamjamir self-assigned this Aug 25, 2018
The :ref:`plugin-info-api-section` is a object that holds all information related to the plugin.
"""
plugin_config_files = hookman_utils.find_config_files(self.plugins_dirs)
return [PluginInfo(plugin_file, self.hooks_available) for plugin_file in plugin_config_files]
Copy link
Member

Choose a reason for hiding this comment

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

You could just add if plugin_info.name not in ignored_plugins to this list comprehension:

return [PluginInfo(x, self.hooks_available) for xin plugin_config_files 
        if x.name not in ignored_plugins]

Also change the signature to accept a Sequence[str]=(), so you don't even need to handle None. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that X is a Path() to where the plugin_config_file is located =/
Only after creating an instance of PluginInfo that I will have access to the content of the configuration (such as name or any other specific ID).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Changed the signture to accept a Sequence[str]=( ) ✔️

hookman/hooks.py Outdated
raise ConflictBetweenPluginsError(
f"Could not get a Hook Caller due to existing conflict between installed plugins")

def get_status(self) -> List[Optional[ConflictStatus]]:
def get_status(self, ignored_plugins: List[str]=None) -> List[Optional[ConflictStatus]]:
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for the signature here: Sequence[str]=().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

hookman/hooks.py Outdated
@@ -154,25 +167,27 @@ def _bind_libs_functions_on_hook_caller(self, shared_lib_path: Path, hook_caller
cpp_func = getattr(hook_caller, hook)
cpp_func(hooks_to_bind[hook])

def ensure_is_valid(self):
def ensure_is_valid(self, ignored_plugins: List[str]=None):
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for the signature here: Sequence[str]=().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

assert len(hm.get_plugins_available()) == 2
assert len(list((datadir / 'plugins').iterdir())) == 2

hook_caller = hm.get_hook_caller(ignored_plugins=['Simple Plugin 2'])
Copy link
Member

Choose a reason for hiding this comment

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

Hmm by caption? We should ignore by some identifier, not the caption...

Copy link
Contributor Author

@williamjamir williamjamir Aug 27, 2018

Choose a reason for hiding this comment

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

I will create an issue to implement this in another PullRequest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#26


plugin_status = hm.get_status()

assert plugin_status
Copy link
Member

Choose a reason for hiding this comment

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

Check the actual status here please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@williamjamir
Copy link
Contributor Author

@nicoddemus
Commits ee85a1d and a33521a apply your review.

Your suggestion about using an identifier instead of caption will be implemented on another PR because I need to do some changes in others modules that was not involved in this PR.

@codecov
Copy link

codecov bot commented Aug 27, 2018

Codecov Report

Merging #25 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #25   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         380    381    +1     
=====================================
+ Hits          380    381    +1

hookman/hooks.py Outdated

def get_hook_caller(self):
plugins_available = [PluginInfo(plugin_file, self.hooks_available) for plugin_file in plugin_config_files]
return [ plugin_info for plugin_info in plugins_available if plugin_info.name not in ignored_plugins]
Copy link
Member

Choose a reason for hiding this comment

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

extra space

@williamjamir williamjamir merged commit cc49168 into master Aug 27, 2018
@williamjamir williamjamir deleted the fb-ASIM-2063-add-option-to-ignore-plugins branch August 27, 2018 16:35
@williamjamir williamjamir restored the fb-ASIM-2063-add-option-to-ignore-plugins branch August 27, 2018 17:09
@williamjamir williamjamir deleted the fb-ASIM-2063-add-option-to-ignore-plugins branch August 27, 2018 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants