-
Notifications
You must be signed in to change notification settings - Fork 28
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
Text with version arg #82
Conversation
version_route = '/matters/{0}/versions' | ||
text_route = '/matters/{0}/texts/{1}' | ||
|
||
versions = self.endpoint(version_route, matter_id) | ||
|
||
latest_version = max( | ||
versions, key=lambda x: self._version_rank(x['Value'])) | ||
if latest_version_value: |
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.
how do we know what the latest_version_value is?
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 find it in the matter dict returned from scraping the API. Like this: https://github.com/opencivicdata/scrapers-us-municipal/blob/optional-arg-text/lametro/bills.py#L295
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 would be better to do that all in this method.
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.
Sure, we could do that by calling matter
, but that adds to the number of times we're hitting legistar, and it duplicates the efforts of the scraper, e.g., the scrape already calls matters
https://github.com/opencivicdata/scrapers-us-municipal/blob/master/lametro/bills.py#L148-L156
Unless...do you have another solution in mind for "doing that all in this method"?
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.
you are right that it adds another call, but it makes the method a lot more intuitive. I think it's probably worth it.
I do think that there's probably a better architecture for this this API along the lines of
class Matter(object):
__init__(self, matter_id):
self._d = request.get(self.matter_endpoint(matter_id))
def text(self):
matter_version = self._d['MatterVersion']
...
return most_recent_tex
This would avoid the duplicative call and reduce the extent of matter_id passing we are doing here.
In any case, that's well beyond the scope of this PR.
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.
reading our code again, I don't thing your proposed interface is that different from our current patterns where the the council specific scraper and the python legistar are both responsible for interacting with the API. I do think a better separation is possible and desirable, but that can wait for another day.
I am not requiring my recommended change.
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.
Oh, poo. I just pushed a revision...actually, could you give your opinion on my latest commit? I think the code is more intuitive, but it assumes that other municipalities use the MatterVersion
field exactly as Metro does. (I'd love to assume that they do, but can we be 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.
i would check to see if _version_rank is definited as an attribute. If it is, then use that.
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.
hmmmmm....all instances of a LegistarAPIBillScraper will have the _version_rank
function, right? So, I'm not sure what you mean by "defined as an attribute."
I am thinking that I should revert to the original state of this PR, which you already approved....
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.
okay
This PR modifies the
text
function in theAPIBillScraper
: the function now enables discovery of the most current bill version via theMatterVersion
field in the API. I included a fairly verbose explanatory doc string, but that verbiage could easier be condensed and/or moved to an issue.This PR relates to Metro-Records/la-metro-councilmatic#237