PR: Command Plugins #100
PR: Command Plugins #100
Conversation
fpliger
force-pushed
the
command-plugins-rd2
branch
from
fc7cf74
to
31978c1
Compare
|
This is not ideal but opens the door for quite a few scenarios... |
|
@havocp would you have some bandwidth to review this PR? |
anaconda_project/plugins.py
Outdated
| return entry_point_plugins | ||
|
|
||
|
|
||
| class ArgsTrasformerTemplate(_ArgsTransformer): |
There was a problem hiding this comment.
would it make sense to have separate files for "stuff plugins would import" and "stuff code that uses plugins would import" ? The second should potentially be in internal/ . Here specifically wondering if get_plugins() is internal and the two Template classes are public API for use by plugin implementations.
There was a problem hiding this comment.
Yeap... That's actually a good separation. Will do in this PR
anaconda_project/project.py
Outdated
| @@ -850,6 +851,11 @@ def _update_commands(self, problems, project_file, requirements): | |||
| first_command_name = None | |||
| commands = dict() | |||
| commands_section = project_file.get_value('commands', None) | |||
|
|
|||
| plugins = plugins_api.get_plugins('command_run') | |||
| all_known_command_attributes_extented = all_known_command_attributes + \ | |||
| @@ -408,6 +408,8 @@ def description(self): | |||
| description = self._attributes.get('windows', None) | |||
| if description is None: | |||
| description = self._attributes.get('conda_app_entry', None) | |||
| if description is None: | |||
| description = getattr(self, 'command', None) | |||
There was a problem hiding this comment.
I originally wrote "why not self.command " but now I'm looking through the class and don't see a command attribute; is somewhere else monkeypatching this in? Maybe there should be a real setter/getter?
There was a problem hiding this comment.
Mmm... in the plugins case self.command is really just a class attr. Is it a name issue (glad to change it in that case) or there was something else?
Overall I'd really like to merge this PR this morning.
There was a problem hiding this comment.
I don't mind the name it was just confusing to me that there's no trace of this attr in the base class. Maybe set it to None in the base? Or make it an instance attr None by default?
There was a problem hiding this comment.
Basically I'd try to have some way this is documented in the base class, if the base class is going to refer to it.
(merge when you want/need to!)
There was a problem hiding this comment.
Basically I'd try to have some way this is documented in the base class, if the base class is going to refer to it.
Oh yup.. that's definitely a miss. Makes sense.
(merge when you want/need to!)
👍 will do as soon as I address the comments and CI is happy :) tnx!
|
Cool. If there are not objections will be merging this soon... |
|
yay! |
This partially implements #89 (at lease for packaged command plugins).
This is a second pass on #94, trimming everything related to "drop in" plugins and just keeping a clean PR for plugins registered using entry_points.