-
Notifications
You must be signed in to change notification settings - Fork 64
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
another usages added #175
base: master
Are you sure you want to change the base?
another usages added #175
Conversation
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.
Thanks for the PR! I had a few questions and suggestions.
Could you also link the issue in the description? (Change Fixes # (issue)
to Fixes #160
, that will link the issue with your PR)
for idx in range(2, len(por)): | ||
if len(por[idx])>1: | ||
site = por[idx] | ||
break |
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.
Question - what does this do? Why not use por[2]
always?
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 initially I thought of that but, if users have a history of local file file:///D...
then the site name will be the empty string.
What do you suggest on this?
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.
A better solution would be to use the in-built urllib.parse
and get hostname
from that. If hostname
is None, we can ignore that tuple.
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
=======================================
Coverage 92.00% 92.00%
=======================================
Files 5 5
Lines 475 475
Branches 87 87
=======================================
Hits 437 437
Misses 26 26
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
Just noticed a few things. However, I will add the hacktoberfest-accepted tag to approve your PR 🎉. Thanks for contributing.
for idx in range(2, len(por)): | ||
if len(por[idx])>1: | ||
site = por[idx] | ||
break |
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.
A better solution would be to use the in-built urllib.parse
and get hostname
from that. If hostname
is None, we can ignore that tuple.
|
||
:: | ||
|
||
from browser_history.browsers import get_history |
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.
Should be
from browser_history.browsers import get_history | |
from browser_history import get_history |
for tup in history: | ||
|
||
url = tup[1] |
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.
It would be slightly more idiomatic to use:
for tup in history: | |
url = tup[1] | |
for _, url in history: |
if site not in frequency: | ||
frequency[site] += 1 | ||
else: | ||
frequency[site] = 1 |
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 conditions are switched :D
if site not in frequency: | |
frequency[site] += 1 | |
else: | |
frequency[site] = 1 | |
if site not in frequency: | |
frequency[site] = 1 | |
else: | |
frequency[site] += 1 |
@ayushkumar1610 do you want to continue working on this? |
Description
Modification made to usages.rst, a use of browser-history is added. A python scripts to show number of times sites visited using graph.
Fixes #160
Type of change
Please delete options that are not relevant.
Checklist: