-
Notifications
You must be signed in to change notification settings - Fork 869
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
Design plugin feature #289
base: master
Are you sure you want to change the base?
Conversation
…from BasePlugin to it
Hi @juped & everyone, Eyitayo, Mwiza, and I have been working on a solution to decompose the howdoi codebase into We'd appreciate feedback on:
|
|
||
from cachelib import FileSystemCache, NullCache | ||
|
||
from pyquery import PyQuery as pq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes it easier to understand this.
from pyquery import PyQuery as pq | |
from pyquery import PyQuery as query_xml |
hyperlinks = element.find('a') | ||
|
||
for hyperlink in hyperlinks: | ||
pquery_object = pq(hyperlink) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think this is a more helpful variable name.
pquery_object = pq(hyperlink) | |
xml_extractor = query_xml(hyperlink) |
for hyperlink in hyperlinks: | ||
pquery_object = pq(hyperlink) | ||
href = hyperlink.attrib['href'] | ||
copy = pquery_object.text() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy = pquery_object.text() | |
copy = xml_extractor.text() |
replacement = copy | ||
else: | ||
replacement = "[{0}]({1})".format(copy, href) | ||
pquery_object.replace_with(replacement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pquery_object.replace_with(replacement) | |
xml_extractor.replace_with(replacement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feedback is great, we'd like to keep this PR a "refactor" and save these valuable renames for a later PR so that we don't change the previous code too much
replacement = "[{0}]({1})".format(copy, href) | ||
pquery_object.replace_with(replacement) | ||
|
||
def get_link_at_pos(self, links, position): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a static method.
except TypeError: | ||
return element.text() | ||
|
||
def _get_search_url(self, search_engine): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _get_search_url(self, search_engine): | |
@staticmethod | |
def _get_search_url(search_engine): |
return [a.attrib['href'] for a in html('.l')] or \ | ||
[a.attrib['href'] for a in html('.r')('a')] | ||
|
||
def _extract_links_from_duckduckgo(self, html): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be a static method
def _extract_links_from_duckduckgo(self, html): | |
@staticmethod | |
def _extract_links_from_duckduckgo(html): |
results.append(parsed_url[0]) | ||
return results | ||
|
||
def _extract_links(self, html, search_engine): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _extract_links(self, html, search_engine): | |
@staticmethod | |
def _extract_links(html, search_engine): |
howdoi/plugins/base.py
Outdated
return filtered_proxies | ||
|
||
def extract(self): | ||
print("Hello extract") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is here.
If it's intended for it to be overriden in child classes, I'd suggest either raising a NotImplementedError
or just leave it as it is (without the print statement).
howdoi/plugins/stackoverflow.py
Outdated
'Safari/536.5'), ) | ||
|
||
|
||
def _random_int(width): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that this just generates a random number, would it be easier to use random.randint
?
} | ||
|
||
|
||
class BasePlugin(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Architecture thoughts:
import enum.Enum as Enum
class Source(Enum):
CLI = 0
DISCORD_MESSAGE = 1
class ServiceConnectionError(Exception):
pass
class Plugin:
name = "UnimplementedPlugin"
def __init__():
pass
def resolve(self):
pass
def extract(self, source, data): # raises an exception if this fails
pass
class PluginStack(list):
def __init__(self, plugins):
super(PluginStack, self).__init__(plugins)
def handle_input(self, source, data):
tries = [ ]
for plugin in self:
try:
plugin.extract(source, data)
plugin.resolve()
except <any of the relevant exceptions>:
tries.append("howdoi could not get a response from {}".format(plugin.name))
Further possible areas for improvement:
- adding
async
support. - "real-time" error handling – i.e. if the first plugin fails, howdoi immediately sends a response
- optional "supports" attribute for
Plugin
s which is a list containing all the sources they support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand PluginStack
. Is it the manager of the plugins installed in howdoi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the idea. Whenver it receives a request it tries each plugin in turn. If a plugin fails, it moves on to the next plugin, otherwise it returns the result of that plugin. The code above isn't 100% complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think the user experience is having the user specify the plugin they want to use with a flag along with their query (at least for now, Hajer is working on a model that automatically detects which plugin is the most appropriate). Do we still need a separate plugin stack class? Maybe if we want to retrieve/update the list of installed plugins...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there has to be a list of installed plugins and a way of resolving/calling them somehow. A class to handle this could still be useful, but it would be a bit different to the PluginStack
.
.vscode/settings.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ | |||
"python.pythonPath": "/Users/cesaredecal/workspace/Environments/howdoi/bin/python" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably shouldn't check this in and instead add it to your .gitignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I forgot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np.
63b79c0
to
0175d81
Compare
No description provided.