-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add default browser support #123
Add default browser support #123
Conversation
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
==========================================
- Coverage 88.07% 87.02% -1.06%
==========================================
Files 3 3
Lines 302 316 +14
Branches 51 53 +2
==========================================
+ Hits 266 275 +9
- Misses 25 29 +4
- Partials 11 12 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@Samyak2 I will not be able to add support for mac OS since I have no access to the platform 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I appreciate you taking the time to find aliases 😄
I have a few comments (in the review below).
Other than that, I had another concern. From what I can see, the default_browser()
function returns only the browser name as a string. From the point of view of an user of the API, the name isn't of much use since it cannot be used to get history of that browser. Instead, it would be better to have it return the browser class directly.
@Samyak2 I will not be able to add support for mac OS since I have no access to the platform 😅
That's okay, we'll have it only for linux for now.
Also, will it be possible to add some basic tests for this? |
It will be tough but I guess we can work it out |
02d145a
to
48cd1cb
Compare
I don't know why the tests are failing in ubuntu and I have wrapped the |
I was thinking of using pytest monkeypatching to set the return values of |
I'll see if it works on my linux system |
The issue was due to
|
Or we can add a |
Also, I have another question. In the issue you said that it was a jungle inside the windows registry 😆 How did you get all the browsers working for windows without mapping out those values? (sorry for all the spam pings xD) |
If we disable import errors we might not be able to capture genuine issues in future, we can just ignore
With the exception of firefox ( which I have handled in a special case ) for the rest we just needed to map the weird alias with no extra processing, since we are not supporting legacy edge and internet explorer (which have inherently sinister names) the rest is easy. Legacy edge doesn't even have a name just random hex values 💀 |
48cd1cb
to
7e61f87
Compare
@Samyak2 If you have any of these browsers installed could you tell me what names they have when using |
That line can be added only for the winreg import, that way pylint would ignore only that import.
I don't have access to a windows system atm, I will try those out tomorrow (IST). |
On linux:
I don't have other browsers currently, I will try them out tomorrow or day after (IST) if that's ok with you. |
I tried it on Mac OS and looks like the object returned by We'll have to skip Mac OS support for now :( |
I guess we can create a new issue for that |
I also don't know how I can fix the failing mypy tests. Is there a way to tell it to ignore the winreg module? |
mypy didn't report any errors when I ran it locally on my system. According to the docs here, we can add
|
f228484
to
a7df77a
Compare
@Samyak2 what's the way forward concerning the testing and monkeypatching involved? Any ideas? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one thing.
I will try writing some tests by EOD, I'm still not sure of the details.
browser_history/__init__.py
Outdated
if browser in default: | ||
return browser_aliases[browser] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause issues if the browser_aliases are expanded in the future by someone else. For example, adding opera
as a key would match operagx
also. Maybe add a warning near browser-aliases
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I was actually thinking of letting the browser implementations provide their aliases through an aliases
attribute which will be a list of all aliases for the specific browser. This will simplify adding new browsers. Maintaining this aliases list is cumbersome. Also, we would need to perform a direct match before looking for deeper searches to try mitigate this issue. For a default value like opera
, we would try finding a direct match with opera
before we match deeper cases like operagx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good, having aliases in the browser classes would be better for maintainability.
Direct matching is also a good idea, but might fail in some cases. For example, if there are operagx
and opera-browser
keys, and the browser name is opera
it might match operagx
. But I understand that this might require some advanced techniques (maybe some NLP?), so checking for a direct match is good enough for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solved the issue using two separate passes (loops). First loop checks for direct matches first therefore, operagx
will be matched with operagx
first no matter what.
The next loop now checks for rough matches. Even though operagx
here would match opera
(the check will assume gx
is just "noise"), it will have already be matched correctly in the first loop so it won't even get to this point.
NLP would be overkill but good; still, I think python's difflib
could do the same job of checking for similarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good. It's probably better to leave enhancements such as using difflib
for a future PR. The current implementation seems quite good! Just needs more testing from the community.
I'll try to finish the tests soon.
8cc464b
to
10884d9
Compare
I see that you have moved the utilities to I have added some basic tests for default browser on Linux and MacOS. Tests for windows will be more difficult as we will have to monkeypatch |
I tested this on Windows and it works reliably for Chrome, Firefox and new Edge. It failed for internet explorer and the old edge, but that was expected. I think this is complete now, except for a few browsers on Linux. |
I will get on this today. I have a slight break from exams. |
It seems like a bug in codecov, where it ignores files if another file of the same name exists in another directory (ref). Since we have |
There is another issue. I tried Microsoft Edge on Linux and it looks like webbrowser does not support it yet. From the source code of webbrowser, I could see that it uses It might be better to use the output of this command directly instead of relying on Sorry for dragging the PR for so long 😅, I want to ensure that everything works before merging. |
I have made everything tidy (Even edge dev on linux is being detected correctly) but I haven't tested it on windows yet. Nothing should be affected by the refactor. I also noticed an unrelated issue where fetching from a browser not supported on the current platform throws an File "/home/obara/projects/python/browser-history/browser_history/cli.py",
line 150, in main cli(sys.argv[1:])
File "/home/obara/projects/python/browser-history/browser_history/cli.py",
line 132, in cli outputs = browser_class().fetch_history()
File "/home/obara/projects/python/browser-history/browser_history/generic.py",
line 139, in __init__ assert self.linux_path is not None, error_string.format("Linux")
AssertionError: Edge browser is not supported on Linux AFAIK this should just be a warning not a full fledged traceback. Maybe create an issue for this since we can't fix it in this feature branch. |
That's great!
I noticed this too, I'll open an issue for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall!
Just a few small things. I will try fixing them today.
@pytest.fixture | ||
def change_linux_default(monkeypatch, request): | ||
"""Changes utils._default_browser_linux to return a specific named | ||
browser. use @pytest.mark.browser_name(name) to set | ||
browser name | ||
""" | ||
|
||
marker = request.node.get_closest_marker("browser_name") | ||
|
||
if marker is None: | ||
browser_name = None | ||
else: | ||
browser_name = marker.args[0] | ||
|
||
def mock_get(): | ||
return browser_name | ||
|
||
monkeypatch.setattr(utils, "_default_browser_linux", mock_get) | ||
|
||
|
||
@pytest.fixture | ||
def change_win_default(monkeypatch, request): | ||
"""Changes utils._default_browser_win to return a specific named | ||
browser. use @pytest.mark.browser_name(name) to set | ||
browser name | ||
""" | ||
if platform != utils.Platform.WINDOWS: | ||
pytest.skip("Skipping windows registry based test") | ||
|
||
marker = request.node.get_closest_marker("browser_name") | ||
|
||
if marker is None: | ||
browser_name = None | ||
else: | ||
browser_name = marker.args[0] | ||
|
||
def mock_get(): | ||
return browser_name | ||
|
||
monkeypatch.setattr(utils, "_default_browser_win", mock_get) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with testing it like this is that _default_browser_linux
and _default_browser_win
are left untested. Since the code in them is very platform-dependent, we should test them thoroughly (ex: if the process returns an empty output, etc.). So we could have separate tests for these.
I'll try to write these tests today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we need to test this deeply, without monkeypatching these complex steps like reading the registry and piping output from the linux command line. I don't know if it can be done.
And the cli tests as well |
22f0647
to
8faf985
Compare
@Samyak2 I noticed Vivaldi support has been added ... we need to support it in defaults as well. |
Right, we need to do that. @OBITORASU would it be possible for you to show us the outputs of |
Also use winreg imports from winreg namespace instead of importing them directly.
8faf985
to
83154d5
Compare
Thank you 👍 @ObaraEmmanuel I think it's all complete now, except for the tests for |
@Samyak2 Sure thing, we've done the most we could. |
Highlighted default browser as a feature in the README since the feature is a bit discreet. Fixed a spacing issues in `aliases :`, moved docstring of `aliases` to the correct position (bottom of it) and fixed some references to `browser_history.generic.Browser`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great work! Thank you for contributing again!
|
||
# first quick pass for direct matches | ||
for browser in all_browsers: | ||
if default == browser.name.lower() or default in browser.aliases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised that since we're using the membership operator here (in
), this will be slightly more efficient if the aliases
are a set instead of a tuple. (ref).
The aliases don't have many elements, so it shouldn't make a big difference. @ObaraEmmanuel what do you think?
Description
This PR adds support for fetching history and bookmarks from default browsers.
Default browser support Tracking
Fixes #96
Type of change
Please delete options that are not relevant.
Checklist: