Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Support the 'electiondate' argument when pulling trend reports #334

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

Conversation

allanjamesvestal
Copy link

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 the BaseTrendReport 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 is api_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 and testresults) 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 of elex.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 and test arguments as in previous versions. Arguments that are common to all the CLI tasks [batch_name, debug, format_json, suppress_output, output_handler_override and with_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 in setup.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

…passed API keys and specific election dates) and to better handle draws for test trend data.
… -- an election date argument. Factor out logic of attaching date to election via CLI, so it can be written once and then used in both date-required and date-allowed decorators.
…o they now consider API key, election date (via the newly-added decorator) and test-result arguments. Change how the trend-file argument gets passed to these.
…h positional args (named args already worked).
… if the '-d/--data-file' argument is also set. This is identical to how the required-date decorator works.
@palewire
Copy link
Contributor

palewire commented Oct 4, 2018

@allanjamesvestal, thank you for this thoughtful patch.

Would it be possible to add a test that verifies a past date is working?

@allanjamesvestal
Copy link
Author

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.

@allanjamesvestal
Copy link
Author

allanjamesvestal commented Oct 4, 2018

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?

@allanjamesvestal
Copy link
Author

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.

@palewire
Copy link
Contributor

palewire commented Oct 6, 2018

Thanks for the additional patches. I'll try to give them a close look in the next few days.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants