Skip to content

Conversation

@mphirke
Copy link
Collaborator

@mphirke mphirke commented Aug 26, 2019

I read Slack and implemented "- Using Newspaper instead of BeautifulSoup [Provides all news scraping capabilities]"

Use of newspaper3k library to extract title instead of beautifulsoup4.
@stephenh67
Copy link

the bs4 import is still included

@asiffarhankhan asiffarhankhan self-requested a review August 26, 2019 18:51
@asiffarhankhan
Copy link
Collaborator

Beautiful soup library does not have any functionality. I will merge this and delete that line on the next commit

@mandjevant
Copy link
Collaborator

Beautiful soup library does not have any functionality. I will merge this and delete that line on the next commit

Nope, the one making the pull request should supply good code with no redundancy and good efficiency. This is to minimize the actions to master directly. Actions directly to master should not be done. Ever.

Removed redudancy.
Removed urllib
@asiffarhankhan
Copy link
Collaborator

@mandjevant sure

@asiffarhankhan asiffarhankhan self-requested a review August 27, 2019 08:24
Reinstated urllib2 and added try and except for internet connection.
Added sys and removed net_con. Directly exits from sys.exit() when the URL is not reachable.
@mphirke
Copy link
Collaborator Author

mphirke commented Aug 27, 2019

I have made the required changes. Please confirm @mandjevant

@mandjevant
Copy link
Collaborator

I have made the required changes. Please confirm @mandjevant

Looks good to me!

@mphirke
Copy link
Collaborator Author

mphirke commented Aug 30, 2019

Can this be merged?

@asiffarhankhan
Copy link
Collaborator

@mphirke i dont think that will be a problem. Cuz I have made all the changes on top of your code. I had copied headline.py from your fork and then added the changes. So as long as other files don disrupt we'll be fine

@asiffarhankhan asiffarhankhan merged commit 01f875f into Learning-Python-Team:master Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants