Skip to content

Huge refactor: entire codebase in PEP8, imports alphabetized, bugfixes, core changes - #71

Merged
codelucas merged 7 commits into
masterfrom
huge-refactor-codelucas
Aug 5, 2014
Merged

Huge refactor: entire codebase in PEP8, imports alphabetized, bugfixes, core changes#71
codelucas merged 7 commits into
masterfrom
huge-refactor-codelucas

Conversation

@codelucas

Copy link
Copy Markdown
Owner

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 of goose by Gravity Labs.
If you compare this line from python-goose: https://github.com/grangier/python-goose/blob/develop/goose/cleaners.py#L216
and this line from goose by gravity: https://github.com/GravityLabs/goose/blob/master/src/main/scala/com/gravity/goose/cleaners/DocumentCleaner.scala#L348

The 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-goose was 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#L153

There 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_class did 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:

  • Adding publishing date extraction
  • Talk of integrating ML into newspaper! Will post more about it soon
  • More fucking unit tests, our testing suite sucks hard

@codelucas

Copy link
Copy Markdown
Owner Author

@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..

@karls

karls commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

@codelucas this is awsome work!

I will take a look over the next couple of hours (hopefully) and comment on this.

Great work man!

@karls

karls commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

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:

  • Is there a reason for the packages directory? Could we pull those packages from PyPi/own repo instead? Pulling out those packages into their own would clean up this project a lot, as well as make the packages themselves a bit better to maintain. Just a thought :-)
  • Could we start using flake8 to ensure non-violation with PEP8?
  • Could hook up TravisCI to run tests and flake8 on master?
  • What's the plan for Python3?

Really good work man!

@codelucas

Copy link
Copy Markdown
Owner Author

@karls:
After thinking about it more you are right, the packages does not really make sense and should be replaced with PyPi imports.

I already use flake8 locally but if we would integrate that into TravisCI
that would be great, i'll read into it (never used Travis before).

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.

@codelucas

Copy link
Copy Markdown
Owner Author

Merging now! The next item on my list is to add a much larger testing suite.

codelucas added a commit that referenced this pull request Aug 5, 2014
Huge refactor: entire codebase in PEP8, imports alphabetized, bugfixes, core changes
@codelucas
codelucas merged commit 25daa2b into master Aug 5, 2014
@karls

karls commented Aug 5, 2014

Copy link
Copy Markdown
Contributor

👍

@codelucas
codelucas deleted the huge-refactor-codelucas branch December 17, 2014 08:27
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.

2 participants