Skip to content
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

Committee notifier #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

geoffkilpin
Copy link
Contributor

Creates a scraper for Parliament's Announcements, Tablings and Committee Reports documents and searches these for announcements of changes to committee membership. If an announcement is found, an email is sent to an address specified in the Pombola configuration file (see mysociety/pombola#1488)

Creates the ATCDocument model, which is largely identical to the Source
model.
Scrapes ATC documents and detects announcements of changes
to committee membership.
self.stdout.write('ATC documents found: %d\nATC documents created: %d\n' % (
sources_count, created_count))

def retrieve_sources(self, start, options):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth factoring out retrieve_sources into a mixin, since it's almost exactly the same in za_hansard_check_for_new_sources, it'd just need the following additional parameters, I think:

  • The category ID for url
  • the source model class (either ATCDocument or Source)
  • the limit

Also, I think it would be worth replacing the recursion of that method with a loop - it doesn't seem worth adding to the stack for each new document.

@mhl
Copy link
Contributor

mhl commented Aug 5, 2014

Hi @geoffkilpin - sorry for not dealing with this for so long, please hassle me if a pull request hasn't been reviewed within a couple of days!

I've left a few comments, the only really significant one is a suggestion to factor out the duplicate code in retrieve_sources. (There are quite a few minor PEP 8 things as well, but I didn't think it was worth mentioning all of those - PyLint should point them out.)

@duncanparkes
Copy link
Contributor

Hi @geoffkilpin, @mhl - I would recommend using flake8 https://pypi.python.org/pypi/flake8 to check for PEP8 type things. PyLint has so much to say it's hard to find the useful bits...

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.

3 participants