-
Notifications
You must be signed in to change notification settings - Fork 67
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
Fetch specified bill from NYC API #182
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.
Like where this is going.
nyc/bills.py
Outdated
self.version_errors = [] | ||
|
||
if matter_id: | ||
matters = [self.fetch(matter_id)] |
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.
I think matter
would be a clearer name.
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.
Is matters
ok given this change?
nyc/bills.py
Outdated
|
||
if raw_option == 'suspended': | ||
continue | ||
def scrape(self, window=3, matter_id=None): |
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.
Do we want to be able to pass in more than one matter_id in at a time?
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.
That's a good question. To me, fetch
implies you're getting one thing. Does it have a similar connotation to you?
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.
Ah, scratch that, I guess there's nothing stopping us from calling fetch
multiple times downstream, eh?
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.
Bearing that in mind, it'd be great to support more than one ID. Good idea.
nyc/bills.py
Outdated
n_days_ago = datetime.datetime.utcnow() - datetime.timedelta(float(window)) | ||
else: | ||
n_days_ago = None | ||
def format_matter(self, matter): |
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.
format_matter isn't quite right is it.... maybe get_bill
?
nyc/bills.py
Outdated
def format_matter(self, matter): | ||
matter_id = matter['MatterId'] | ||
if matter_id in DUPLICATED_ACTIONS: | ||
return |
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.
let's explicitly return a None
here and everywhere else
nyc/bills.py
Outdated
except KeyError: | ||
version_errors.append(legistar_web) | ||
continue | ||
def format_event(self, bill, action): |
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.
+1 on breaking this into multiple methods. Let's find a better name than format_event
nyc/bills.py
Outdated
except KeyError: | ||
version_errors.append(legistar_web) | ||
continue | ||
def get_vote_event(self, bill, action): |
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.
This doesn't seem like the quite right separation.
when you do
act = bill.add_action(action['action_description'],
action['action_date'],
organization={'name': action['responsible_org']},
classification=action['classification'])
does the act
object have a handle on the bill? if so, could you just pass in an act
?
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.
We don't have a handle on the vote events there, but I agree the separation is a bit confusing. How about c9d9f37?
nyc/bills.py
Outdated
organization={'name': action['responsible_org']}, | ||
classification=action['classification']) | ||
if raw_option == 'suspended': | ||
return None |
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.
this should be a continue
I'm pretty sure.
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.
great catch!
nyc/bills.py
Outdated
'''Make VoteEvent object from given Bill, action, and vote.''' | ||
result, votes = vote | ||
|
||
if result: |
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 think I would be this check outside the method and just pass in votes.
Related to opencivicdata/python-legistar-scraper#59
Allows user to pass in
matter_id
to update a specific bill, rather than having to update all the bills in relevant time period.