adds officeid filter; closes #320 #326
Conversation
CHANGELOG.rst
Outdated
| @@ -1,3 +1,8 @@ | |||
| 2.4.1 (hold) - June 13, 2018 | |||
There was a problem hiding this comment.
Typically repo owners will do publishing and versioning; as a feature creator you don't need to edit CHANGELOG or equivalent files, so I'd remove them from the PR.
docs/cli.rst
Outdated
| @@ -88,7 +88,8 @@ Commands and flags | |||
| --batch-name BATCH_NAME | |||
| Specify a value for a `batchname` column to append to | |||
| each row. | |||
|
|
|||
| --officeids OFFICEIDS Specify officeids to parse | |||
There was a problem hiding this comment.
I'd put this higher up in the order, nearer to similar settings like --results-level and --raceids. Think of the UX from the perspective of a new dev learning the settings from elex --help.
elex/api/models.py
Outdated
| @@ -876,6 +876,7 @@ def __init__(self, **kwargs): | |||
| self.setzerocounts = kwargs.get('setzerocounts', False) | |||
|
|
|||
| self.raceids = kwargs.get('raceids', []) | |||
| self.officeids = kwargs.get('officeids', "") | |||
There was a problem hiding this comment.
Add a comment here to explain why officeids are being handled as a string instead of a list of strings; this may not be clear to future devs.
There was a problem hiding this comment.
Also, should the default value be "", or None? I genuinely don't know; you have a better grasp on how requests will submit this querystring.
There was a problem hiding this comment.
On second comment - After poking around a little more, I don't think it matters at this particular point because this particular line is executed before the command line args are parsed. But I do think None is a cleaner default value!
| @@ -98,7 +98,6 @@ def api_request(path, **params): | |||
| params = sorted(params.items()) # Sort for consistent caching | |||
|
|
|||
| url = '{0}{1}'.format(elex.BASE_URL, path.replace('//', '/')) | |||
|
|
|||
There was a problem hiding this comment.
No need to make this change to a file you aren't otherwise editing; think this is probably here because of a debug print we were doing earlier.
elex/cli/app.py
Outdated
| @@ -90,6 +90,11 @@ class Meta: | |||
| action='store', | |||
| help='Specify a value for a `batchname` column to append to each row.', | |||
| )), | |||
| (['--officeids'], dict( | |||
| action='store', | |||
There was a problem hiding this comment.
Why store instead of store_true? I haven't looked into what action does, would be curious.
There was a problem hiding this comment.
store stores officeids specified by the user and has no default value like store_true does. store would issue an error before executing any code if a user tried to run elex races 2016-11-08 --officeids and failed to specify office IDs because arguments are required if the --officeids flag is included. store_true would in this example result in officeids being a True, which would cause the code to fail when it tries to parse officeids. Here's more info!
elex/cli/app.py
Outdated
| @@ -90,6 +90,11 @@ class Meta: | |||
| action='store', | |||
| help='Specify a value for a `batchname` column to append to each row.', | |||
| )), | |||
| (['--officeids'], dict( | |||
| action='store', | |||
| help='Specify officeids to parse', | |||
There was a problem hiding this comment.
The other options' help text ends in a period.
elex/cli/app.py
Outdated
| (['--officeids'], dict( | ||
| action='store', | ||
| help='Specify officeids to parse', | ||
| default=[] |
There was a problem hiding this comment.
Default to [] or "" or None?
elex/cli/hooks.py
Outdated
| @@ -28,6 +30,14 @@ def add_election_hook(app): | |||
| if app.pargs.raceids: | |||
| app.election.raceids = [x.strip() for x in app.pargs.raceids.split(',')] | |||
|
|
|||
| if app.pargs.officeids: | |||
| invalid_officeids = [x.strip() for x in app.pargs.officeids.split(',') if x.strip() not in maps.OFFICE_NAMES] | |||
There was a problem hiding this comment.
Why are the values being stripped? It doesn't look like the processing of the office IDs elsewhere in the code is being stripped. But I'm not super familiar with arg parsing, so defer to you if this is necessary.
There was a problem hiding this comment.
Nope, you're right. It's unnecessary, particularly since adding a bunch of spaces in front of P,H,G in elex races 2016-11-08 --officeids P,H,G doesn't cause officeids to reflect those spaces, and elex races 2016-11-08 --officeids P, H, G errors out and doesn't run.
elex/cli/hooks.py
Outdated
| text = '{0} is/are invalid officeID(s). Here is a list of valid officeIDs: {1}' | ||
| app.log.error(text.format(", ".join(invalid_officeids), ", ".join(maps.OFFICE_NAMES.keys()))) | ||
| app.close(1) | ||
| app.election.officeids = app.pargs.officeids |
There was a problem hiding this comment.
Maybe stash this in an else: clause, would make it read a bit easier to me.
tests/__init__.py
Outdated
| testresults=False, | ||
| liveresults=True, | ||
| is_test=False, | ||
| officeids='N,ABC,PS' |
There was a problem hiding this comment.
I'd change this test to not use multi-letter fake office IDs, to make it more reasonable.
vv-qian
changed the title
|
@palewire Ready for you! |
No description provided.