Skip to content
This repository has been archived by the owner on Oct 25, 2019. It is now read-only.

Fix several broken modules, remove mlia, skip vtluugwiki tests until it's back up #70

Merged
merged 10 commits into from Feb 12, 2017

Conversation

paulwalko
Copy link
Collaborator

Several search modules needed updating, also added duckduckgo api as preferred method.
Fixed bad weather links.
Removed mlia module since the since seems to be down indefinitely.
Skipped vtluugwiki tests until wiki is back up.

Copy link
Owner

@mutantmonkey mutantmonkey left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I have a few changes I'd like you to make and then I'd be happy to merge. There's no need to squash or anything; you can just commit to the same branch.

@@ -6,7 +6,7 @@

import random

otherbot = "truncatedcone"
otherbot = "quone"
Copy link
Owner

Choose a reason for hiding this comment

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

It might be good to make this a config option in the future, actually. There's no need to do it now, though.

modules/linx.py Outdated
if len(data) <= 0 or not data['success']:
phenny.reply('Sorry, upload failed.')
return
url = url.replace(".onion/", ".onion.to/")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather not do this. I understand the benefit, but I don't really like relying on random tor2web gateways.

An idea I've had for a while is the ability to configure phenny so that outgoing HTTP/HTTPS requests go over a proxy that can be configured separately than the proxy used to connect to IRC. Now that requests has SOCKS support, this shouldn't be particularly difficult. I'd prefer telling people to do that, rather than using a tor2web gateway. Perhaps the configuration could even work on a per-module basis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I remove this line?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes please.

print(results)
return False
query = web.quote(query)
uri = 'https://google.co.uk/search?q=%s' % query
Copy link
Owner

Choose a reason for hiding this comment

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

I'm surprised this works and Google isn't blocking it. Any reason you're using google.co.uk over google.com?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't matter to me, I just saw that bing and ddg used en-GB, so I assumed you'd want google to as well.

@@ -125,22 +121,34 @@ def bing(phenny, input):
bing.commands = ['bing']
bing.example = '.bing swhack'

r_duck = re.compile(r'nofollow" class="[^"]+" href="(http.*?)">')
r_duck = re.compile(r'nofollow" class="[^"]+" href=".+?(http.*?)">')
Copy link
Owner

Choose a reason for hiding this comment

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

Can you provide an example of when this is necessary? It seems strange...

Copy link
Collaborator Author

@paulwalko paulwalko Feb 12, 2017

Choose a reason for hiding this comment

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

I don't exactly know how ddg's search has changed so it's hard to explain why that addition is needed, but here's what it's getting (searching for 'ayy'):

<a rel="nofollow" class="result__a" href="/l/?kh=-1&amp;uddg=http%3A%2F%2Fwww.urbandictionary.com%2Fdefine.php%3Fterm%3DAyy">

I suppose that could be changed with '.*' or '.+', but '.+?' was just the first thing I found to get it to work so I kept it.


uri = duck_search(query)
uri = duck_api(query)
if not uri: uri = duck_search(query)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to have these on separate lines.

@@ -28,15 +28,15 @@ def validate(actual_name, actual_lat, actual_lon):
('27959', check_places("Dare County", "North Carolina")),
('48067', check_places("Royal Oak", "Michigan")),
('23606', check_places("Newport News", "Virginia")),
('23113', check_places("Midlothian", "Virginia")),
('23113', check_places("Nodstown", "Munster")),
Copy link
Owner

Choose a reason for hiding this comment

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

Well...it was supposed to be Midlothian. Could you just remove this test instead? If the Nominatim changes again it could end up back to being Midlothian.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, I think I was playing around with tests trying to get them to work and forgot to come back to this one. I'll remove it.

@paulwalko
Copy link
Collaborator Author

paulwalko commented Feb 12, 2017

I noticed wiktionary is broken for the vast majority of terms I tried, so I'll probably fix that later.

Also, in my personal fork (github.com/paulwalko/phenny) I made a quote module that saves quotes for a website to display. Would you merge that into your fork, or should I keep it separate?

@mutantmonkey
Copy link
Owner

Sure, feel free to submit the quote module as well. I'd prefer that as a separate PR so we don't hold this one up, though.

@paulwalko
Copy link
Collaborator Author

Great, I still need to fix some bugs with the quote module anyways.

@mutantmonkey mutantmonkey merged commit e4d28c0 into mutantmonkey:master Feb 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants