Added -b or --browser parameter #17
Conversation
pySearch.py
Outdated
| def openBrowser(self): | ||
| webbrowser.open_new_tab(self.url) | ||
| try: | ||
| browser = webbrowser.get(args.browser[0]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will continue to develop this with the specified functionalities, thanks for the recommendations!
|
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: The script currently works assuming web browsers are registered as their executable file name (chrome, firefox, safari, iexplore) |
jrkong
left a comment
jrkong
left a comment
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
|
I think we can add a disclaimer for the Could you add the command line argument to the README and add a statement or warning regarding Windows functionality? |
There was a problem hiding this 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
| <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> |
There was a problem hiding this comment.
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?
|
Hello @JoshuaRM! Thanks for updating the PR.
|
|
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
left a comment
jrkong
left a comment
There was a problem hiding this comment.
You forgot to add the -b argument to the code after you grabbed the updates from the test framework.
| argparser.add_argument("-d", "--domain", | ||
| help="Changes the domain extention", | ||
| nargs="+") | ||
|
|
There was a problem hiding this comment.
You forgot to add your -b argument here.
| - `-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. |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
3 participants
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