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

Added -b or --browser parameter #17

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@JoshuaRM
Copy link

commented Nov 11, 2018

Solves #14 Added a -b parameter that allows the user to specify which browser to open in.
If an invalid or unregistered browser is entered, an error is shown and the available list of browsers is displayed

pySearch.py Outdated
@@ -44,7 +52,11 @@ def buildLink(self):
#end of link building

def openBrowser(self):
webbrowser.open_new_tab(self.url)
try:
browser = webbrowser.get(args.browser[0])

This comment has been minimized.

Copy link
@jrkong

jrkong Nov 11, 2018

Owner

Great first start but this is unsafe. There is no default state if the browser argument isn't supplied and overall I would avoid handling the browser argument in openBrowser as it makes the function harder to test with a test suite. Instead could you create a handler for browser arguments(setBrowser perhaps)?

If the user doesn't input a browser then you don't need to run webbrowser.get. As it is right now your solution makes the -b argument mandatory. Look at how the arguments for engine (on line 58) and domain (line 61) are handled when they don't receive any arguments.

If possible I would like to see this handler implement a bit of logic to allow aliases for the different browsers (so I can call google chrome by typing Chrome, google or some other shorthand). Something like this could be achieved with a regular expression.

If I wanted the firefox browser the regular expression [Ff]irefox would allow caps for firefox or using ([Gg]oogle)|([Cc]hrome)|([Gg]oogle [Cc]hrome) would allow the google alias to map to the preregistered chrome browser. This would mitigate some degree of user error and I think it would be a great quality of life improvement.

This comment has been minimized.

Copy link
@JoshuaRM

JoshuaRM Nov 12, 2018

Author

I will continue to develop this with the specified functionalities, thanks for the recommendations!

@JoshuaRM

This comment has been minimized.

Copy link
Author

commented Nov 12, 2018

Updated the browser parameter to a safer state. The parameter has been created to work with chrome, firefox, internet explorer, and safari, and has been tested on both Mac OS and Windows.

The parameter uses regular expression and accepts any of the following terms, all case insensitive:
To open in chrome
google, chrome, google chrome, google-chrome
To open in firefox
firefox, mozilla-firefox, mozilla, mozilla firefox
To open in internet explorer
ie, internet-explorer, internet explorer, iexplore, iexplorer
To open in safari
safari

The script currently works assuming web browsers are registered as their executable file name (chrome, firefox, safari, iexplore)

@jrkong
Copy link
Owner

left a comment

I like what you've got overall but there's one style nit that really stands out for me. I've also added a comment on suggestions on how I think we should handle browser registration on the issue. Though, after that's settled I think this PR will be ready to be merged.

pySearch.py Outdated
#print available broswers
def printAvailableBrowsers(invalid):
print("You have selected an invalid or unreigstered browser: " + invalid + ".\nHere is a list of available browsers")
for i in webbrowser._browsers:

This comment has been minimized.

Copy link
@jrkong

jrkong Nov 13, 2018

Owner

Code nit, let's avoid using a generic iterator like i. Iterators in Python's for loops act as the actual object so I think we should use a variable that's more informative to maintain the readability of the code. i in particular is an incredibly shaky choice when i is the generic int iterator variable in other languages. I'm worried that someone new to Python may look at the code and assume i is an int.

Instead for browser in webbrowser._browsers: gives a much better indication of what's going on.

@jrkong

This comment has been minimized.

Copy link
Owner

commented Nov 16, 2018

I think we can add a disclaimer for the -b command line argument stating that it requires the browser path to be in the user's PATH to work properly on Windows.

Could you add the command line argument to the README and add a statement or warning regarding Windows functionality?

@jrkong
Copy link
Owner

left a comment

Could you resolve these conflicts and verify that everything passes the flake8 tests?

The conflicts occur because the README was updated to use just markdown syntax and pySearch.py got a bit of structure added to it for the pip update. This should only involve moving your logic into the main function.

README.md Outdated
@@ -34,6 +34,7 @@ python C:\Users\pySearch\pySearch.py ( different path name where you have clone
<li> -e changes name or alias of search engine and sets it to as search engine for session </li>
<li> -d changes to domain extension </li>
<li> -h will provide description of command line arguments </li>
<li> -b changes the browser to search in. If an invalid browser is selected, a list of valid ones will be displayed.</li>

This comment has been minimized.

Copy link
@jrkong

jrkong Dec 7, 2018

Owner

Could we mention that on Windows, browsers are only registered IF the path for the browser's install directory is added to the system's environment variables?

@pep8speaks

This comment has been minimized.

Copy link

commented Dec 8, 2018

Hello @JoshuaRM! Thanks for updating the PR.

  • In the file pySearch.py, following are the PEP8 issues :

Line 11:28: W503 line break before binary operator

  • There are no PEP8 issues in the file search.py !
@JoshuaRM

This comment has been minimized.

Copy link
Author

commented Dec 8, 2018

In this commit I changed the flake8 ignore list to add the flake8 error it gives now. The binary operator needs to be on separate lines, and flake8 does not like that.

@jrkong
Copy link
Owner

left a comment

You forgot to add the -b argument to the code after you grabbed the updates from the test framework.

nargs="+")
argparser.add_argument("-d", "--domain",
help="Changes the domain extention",
nargs="+")

This comment has been minimized.

Copy link
@jrkong

jrkong Dec 12, 2018

Owner

You forgot to add your -b argument here.

@@ -24,6 +24,9 @@ For Windows: Command Line Prompt
- `-e` changes name or alias of search engine and sets it to as search engine for session
- `-d` changes to domain extension
- `-h` provides a description of each command line argument
- `-b` changes the browser to search in. If an invalid browser is selected, a list of valid ones will be displayed.

Note for Windows users: browsers that are not added to the system environment variables will not be usable, or recognized as an available browser.

This comment has been minimized.

Copy link
@jrkong

jrkong Dec 12, 2018

Owner

Could you put this under a sub-bullet of -b?

try:
webbrowser.get(self.browser).open_new_tab(self.url)
except(webbrowser.Error):
printAvailableBrowsers(self.browser)

This comment has been minimized.

Copy link
@jrkong

jrkong Dec 12, 2018

Owner

I'm debating if we should open the search in the default browser if the selected browser is invalid. This could be resolved in another issue if necessary but this is something that's worth thinking about.

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.