Open allanjamesvestal opened 6 years ago
@allanjamesvestal, thank you for this thoughtful patch.
Would it be possible to add a test that verifies a past date is working?
Can do! I'll add that in this afternoon.
As I mentioned in the tome above, I have a second branch that fixes a couple pre-existing flake8 issues. It'd be a little time-consuming to extract those from the changes here, so I'd like to do that second if this is going to be able to be merged somewhat quickly.
I did want to mention it here, though, because you may want to group the two together in the same version bump.
Hey @palewire, so I'm writing that test now. It's a little more involved than the other tests around trend reports (it's a two-step process that involves getting the list of all reports, and then getting the actual report we want), so I wanted to run this by you.
The good news is that I can pretty easily just patch
the function that wraps the requests
library and have it spit back the right JSON in each place.
The bad news is, that all requires Mock to be installed. Well, it's there for Python 3, but I'll need to add a line to requirements-dev
to get it installed for anyone still on Python 2 (maybe PyPy as well).
Is that OK?
Updated with two tests — one to ensure calling the loader without an electiondate
set gets the latest report, another to verify setting electiondate
does what it should.
They and all other tests pass for me under Python 2 & 3 and PyPy.
Thanks for the additional patches. I'll try to give them a close look in the next few days.
This branch adds a couple configuration options to
elex.api.trends.BaseTrendReport
, and also to the corresponding CLI tasks (elex.cli.app.ElexBaseController.{governor|house|senate}_trends
).Chief among these additions is support for an
electiondate
option in theBaseTrendReport
constructor, which filters the returned list of reports by election-date value. That enables users to get balance-of-power reports for several past elections. (The other added option isapi_key
for explicit key settings -- since the AP has required organizations including ours to use separate keys for national and state-by-state data in the past.)All arguments passed to the
BaseTrendReport
constructor now come in as*args, **kwargs
, which adds future flexibility. Consequently, the parameters that were already supported (trend_file
andtestresults
) retain their names and default values, but the logic by which they are set changes slightly.The
{governor|house|senate}-trends
CLI commands have been modified to accept — but not require — a date value. In prior releases, a date value wouldn't raise an error, but would be ignored. This date support has been added by creating a new CLI-command decorator that mimics the behavior ofelex.cli.decorators.require_date_argument
, but without erroring out if no date was set.(These three CLI commands continue to support the optional
trend_file
andtest
arguments as in previous versions. Arguments that are common to all the CLI tasks [batch_name
,debug
,format_json
,suppress_output
,output_handler_override
andwith_timestamp
] have all been tested and should still work properly as well.)These changes have been tested* using
make test
in Python 2.7.8 and 3.6.5, as well as in the latest Homebrew-installed PyPy. However, I haven't written any additional tests or documentation for these — nor have I bumped any version numbers insetup.py
or elsewhere (I wanted feedback on what's needed before I did so).Please let me know how these changes look. I'd love to get them incorporated before the election, if it's possible.
make test
actually fails due to a few linting issues that pre-date my forking of the repo. I've resolved them in a branch on my own forked repo, and will submit a second PR with these minor changes when/if this current PR gets merged