Huge refactor: entire codebase in PEP8, imports alphabetized, bugfixes, core changes - #71
Conversation
Also fixed spacing between imports and body code if needed.
…m params, not article or source objects
…m params, not article or source objects
…s/newspaper into huge-refactor-codelucas Conflicts: newspaper/extractors.py
|
@karls, @jeffnappi, you two seem active on this topic, anyone interested in looking this over? Maybe there should be a commit which updates a bunch of new unit tests before this gets merged.. |
|
@codelucas this is awsome work! I will take a look over the next couple of hours (hopefully) and comment on this. Great work man! |
|
It's looking great I think! I'm a little concerned about such a big refactor with so few tests, but there's not much we can do. Some other thoughs:
Really good work man! |
|
@karls: I already use As for py3, there have been talks about it: #38 I've not given much thought to it because there are other items on the roadmap which are more valuable. But this should be given more attention.. A large part of this is also having all of newspapers deps be python 3 compatible. |
|
Merging now! The next item on my list is to add a much larger testing suite. |
Huge refactor: entire codebase in PEP8, imports alphabetized, bugfixes, core changes
|
👍 |
I'd like an extra pair of eyes on this pull request before it gets merged. The testing suite and a few dry runs work just fine but there are not enough tests to cover every code path.
This was meant to be a huge refactor (1K lines updated) but there were also quite a few bugfixes & core changes
that happened.
Some notable lines that have been changed:
A portion of this codebase was taken from
python-goose, a python port ofgooseby Gravity Labs.If you compare this line from
python-goose: https://github.com/grangier/python-goose/blob/develop/goose/cleaners.py#L216and this line from
gooseby gravity: https://github.com/GravityLabs/goose/blob/master/src/main/scala/com/gravity/goose/cleaners/DocumentCleaner.scala#L348The line was ported over wrong, so our fix:
https://github.com/codelucas/newspaper/blob/huge-refactor-codelucas/newspaper/cleaners.py#L178
Also, a large chunk of
python-goosewas written in an unusual style where huge objects were being passed around everywhere instead of the properties of those objects which were used. This resulted in a very illegible codebase (It's still a great library).Look at our changes, see how the cleaners, extractors, outputformatters now take explicit values and not
self: https://github.com/codelucas/newspaper/blob/huge-refactor-codelucas/newspaper/article.py#L153There were large and unnoticed bugs in newspaper prior to this PR.
If a language is not set via configs or inputs, newspaper will attempt to detect the language to use (from meta tags). This worked fine for all latin languages (en, fr, etc) because the
stopwords_classdid not need to be updated. However, for languages like Arabic or Chinese, this would fail. This has been fixed as well.The codebase has been updated to fulfill PEP8 standards, imports have also been alphabetized for readability. There were also some other misc bugs laying around but that's not important. I didn't do a good job at all of splitting up my commits, this was rushed over the weekend sorry!
On the roadmap for newspaper: