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

Conversation

Projects
None yet
2 participants
@alexander-ponomaroff
Copy link
Contributor

commented Nov 13, 2018

Checks if the current domain works with the search engine that the user selected.

If not, looks through popular domain engines and selects the first working one.

#9

alexander-ponomaroff added some commits Nov 13, 2018

@jrkong
Copy link
Owner

left a comment

Take a look at my comment on the issue. Let's avoid ping because it doesn't check the search string.

There's a lot of unnecessary new lines can we remove those as well?

Show resolved Hide resolved pySearch.py Outdated
Show resolved Hide resolved pySearch.py Outdated
Show resolved Hide resolved pySearch.py Outdated
@alexander-ponomaroff

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@jrkong Yeah I will be making changes based on what we discussed. Like I said I wanted to make a pull request with already a working version that I made and I will be working to improve it this week.

@alexander-ponomaroff

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@jrkong I've redone everything to use Invoke-RestMethod for windows and curl call for mac and linux. I tested all the cases and they work on my mac. Could you please test on windows and let me know if it works on windows. Thanks.

@jrkong
Copy link
Owner

left a comment

Could you update your PR so the code works with the newly implemented test framework?

@jrkong
Copy link
Owner

left a comment

Looks good so far but there are a few flake8 errors.

@jrkong
Copy link
Owner

left a comment

@alexander-ponomaroff bad news. curl seems relatively uncooperative in tests so this implementation doesn't work. So I think we should bring in external libraries since it seems unavoidable at this point.

I did some quick tests and the requests library seems like the cleanest way to implement what we need. We can also use the urllib or httplib but I think the requests implementation is easiest to read.

To perform the url ping/call using requests use requests.head(self.url). This call returns an object. Assuming the object is stored in a variable you can then reference the status code through .status_code.
So the call to a site would look something like this:

r = requests.head(self.url)
if r.status_code != 200 and r.status_code != 301:

So this code chunk contains a few things I found during my tests: requests.head asks for the header of the site at the URL which is enough to test if the site exists.

Status code 301 is checked as some search engines will return a 301 for a search URL when called through a CLI interface. For an example of this try running curl -X POST "https://stackoverflow.com/search?q=test".

Also you may have to perform this in a try block as requests may throw an exception if it maxes out it's retry limit. During my tests a case that triggered this was https://stackoverflow.cn/search?q=test.

Show resolved Hide resolved search.py Outdated
Show resolved Hide resolved search.py Outdated
@alexander-ponomaroff

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@jrkong I just fixed the Flake8 issues. This is very weird because when I previously worked with curl, it worked perfectly, but now that I'm running the program again, I'm running into issues as well. I will take a look if anything can be done to fix the issues with curl. If not, I will look into the suggested requests library and will push the changes within the next couple days.

@jrkong

This comment has been minimized.

Copy link
Owner

commented Dec 20, 2018

Sure. I'm confused as well, I remember that it worked previously as well but something seems to have changed that and I'm not quite sure what it is. I'm slightly concerned that this may be a reoccurring issue which is why I suggested the switch. Regardless, I've been doing research into the cause as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.