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.
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 |
ObaraEmmanuel
changed the title
ObaraEmmanuel
force-pushed
the
feature-default
branch
from
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 💀 |
ObaraEmmanuel
force-pushed
the
feature-default
branch
from
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
|
ObaraEmmanuel
force-pushed
the
feature-default
branch
2 times, most recently
from
f228484
to
a7df77a
Compare
|
@Samyak2 what's the way forward concerning the testing and monkeypatching involved? Any ideas? |
browser_history/__init__.py
Outdated
| if browser in default: | ||
| return browser_aliases[browser] |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
ObaraEmmanuel
force-pushed
the
feature-default
branch
from
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. |
|
The code coverage seems to have decreased because Codecov doesn't recognize Also, we should add some default browser tests in CLI too: |
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 LinuxAFAIK 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. |
| @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.
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.
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 |
ObaraEmmanuel
force-pushed
the
feature-default
branch
from
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 |
|
Yes I can show the output of Branch: feature-default The output for Image attached: |
Also use winreg imports from winreg namespace instead of importing them directly.
ObaraEmmanuel
force-pushed
the
feature-default
branch
from
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`.
|
|
||
| # 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.
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: