Rough in search method #67
Conversation
|
@hancush this is ready for your review |
legistar/base.py
Outdated
| Arguments: | ||
|
|
||
| route -- The path to search, i.e. /matters/, /events/, etc | ||
| item_key -- The unique id for the items that you are searching. |
There was a problem hiding this comment.
I'm not sure I understand what this means.
tests/test_search.py
Outdated
| from legistar import base | ||
|
|
||
|
|
||
| class TestAPISearch(unittest.TestCase): |
There was a problem hiding this comment.
Why use unittest here, instead of pytest?
There was a problem hiding this comment.
This is what pytest says to do? https://docs.pytest.org/en/latest/unittest.html
There was a problem hiding this comment.
It seems like that's what pytest says to do "for leveraging existing unittest-based test suites." Since legistar doesn't have an existing test suite (unless I missed it?), I think it'd be great to just use pytest conventions, with a fixture for setup in conftest, that's included in each test. You can still use classes to organize tests, if you like.
There was a problem hiding this comment.
@fgregg Is this something you want to do, in this PR?
|
@hancush okay, updated the docstring. let me know if that helps. |
legistar/base.py
Outdated
| and item key in a wrapper function. | ||
|
|
||
| If you were searching for matters, then this would looke like | ||
| search('/matters/', 'MatterId', your_search_condition) |
There was a problem hiding this comment.
I think the sentence about wrapper functions is a little confusing. May I suggest striking it, in lieu of a format like:
"""
Base function for searching the Legistar API.
Arguments:
blah
Examples:
# Search for bills introduced after Jan. 1, 2017
search('/matters/', 'MatterId', "MatterIntroDate gt datetime'2017-01-01'")
"""
|
okay @hancush updated the docstring again. |
|
@hancush okay, using pytest conventions. |
tests/test_search.py
Outdated
|
|
||
| from legistar import base | ||
|
|
||
| @pytest.fixture(scope="module") |
There was a problem hiding this comment.
I'd break this fixture out into test/conftest.py, that way it will be available if we want to add more test modules later. (FWIW, if you do that, you don't need to import fixtures from conftest, here.)
There was a problem hiding this comment.
That seems like something we can do once we have more tests? Or is it a strong DataMade convention that fixtures should only go in conftest?
There was a problem hiding this comment.
I would argue that it's a convention. A centralized location for generic fixtures, like this one, lowers the barrier to writing additional test modules moving forward (because that fixture will be available for use in those modules). If there fixtures are truly specific to a test module, ok – but this one seems reusable beyond testing one method on the base scraper.
There was a problem hiding this comment.
I think it makes this thing a bit less readable, but conventions are important. I've made the change.
Is there anything else?
There was a problem hiding this comment.
This seems fine to me!
Related to Metro-Records/la-metro-councilmatic#263, adds a method to search the Events API.
TO DO
endpointto the baseLegistarAPIScraperclass. (This is also a step toward Add a method for scraping specific bills and events #59.)partialmethods in bills scraper, ifendpointis not defined in-file?python-legistar-scraper/legistar/bills.py
Lines 259 to 260 in 63d47af