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

Fix issue #9 - Ensure that domain works #20

Open
wants to merge 9 commits into
base: master
from
@@ -3,6 +3,8 @@
import urllib
import argparse
import webbrowser
import platform
import subprocess

argparser = argparse.ArgumentParser()
argparser.add_argument("-s", action="append", help="Takes a query to search for and searches it.", nargs="*")
@@ -45,9 +47,32 @@ def buildLink(self):
else:
self.searchQuery = "+".join(self.searchRaw)
#end of search exceptions
print(self.ping())
self.url = "http://www." + self.engine + "." + self.domain + self.searchString + self.searchQuery
#end of link building

def ping(self):

popularDomains = ["ca", "com", "de", "cn", "net", "uk", "org", "info",
This conversation was marked as resolved by alexander-ponomaroff

This comment has been minimized.

Copy link
@jrkong

jrkong Nov 13, 2018

Owner

I like this however can we separate the ping functionality and check other domains functionality?

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Nov 13, 2018

Author Contributor

I'll put the two functionalities in separate functions.

"nl", "eu", "ru"]

# Checks if the operating system is windows
param = '-n' if platform.system().lower()=='windows' else '-c'

command = ['ping', param, '1', self.engine + "." + self.domain]

if subprocess.call(command) != 0:

for dom in popularDomains:

command = ['ping', param, '1', self.engine + "." + dom]

if subprocess.call(command) == 0:
self.domain = dom
return "The domain you entered didn't work! Here's one that works: " + dom
This conversation was marked as resolved by alexander-ponomaroff

This comment has been minimized.

Copy link
@jrkong

jrkong Nov 13, 2018

Owner

I like this but lets change the message a little. Reading the output message as it is right now seems to imply that the user has to fix their inputs. This should not be the case. pySearch should be smart enough to change the URL for the user and open the final URL. It feels unintuitive to have the user retype their search properly if we can detect and build a valid URL.

I would suggest returning the URL to buildLink and inside buildLink check if the original URL and returned URL match. If they don't match then output a message that tells the user (_original url_ is invalid, opened _new url_ instead). This way there's no disruption for the user.

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Nov 13, 2018

Author Contributor

It does not make the user retype the command, it already opens the link with the new domain. The message does need to be changed though.

else:
return "The domain you entered worked!"
This conversation was marked as resolved by alexander-ponomaroff

This comment has been minimized.

Copy link
@jrkong

jrkong Nov 13, 2018

Owner

I believe we don't need a return if a valid URL is detected. Let's keep outputs to a minimum if everything is working correctly. A browser opening the appropriate tab(s) is enough feedback.

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Nov 13, 2018

Author Contributor

Agreed, I had the text feedback for testing.


def openBrowser(self):
webbrowser.open_new_tab(self.url)
#end of openBrowser()
@@ -66,3 +91,4 @@ def openBrowser(self):
searchObj.setQuery(search)
searchObj.buildLink()
searchObj.openBrowser()

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.