Skip to content
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

don't enum motion_classification, as it makes votes useless for non-Sunlight projects #149

Merged
merged 1 commit into from
Feb 24, 2015

Conversation

jpmckinney
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.06% when pulling 8731e79 on jpmckinney:voteclass into 4a933f2 on opencivicdata:master.

@@ -7,7 +7,7 @@
"properties": {
'identifier': {"type": "string", "blank": True,},
'motion_text': {"type": "string" },
'motion_classification': {"items": {"type": "string", "enum": common.VOTE_CLASSIFICATIONS},
'motion_classification': {"items": {"type": "string"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hurm, I'm not super sure if we want to entirely get rid of all checking here, but I see what you're saying.

Hurmm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Sunlight needs to ensure motion_classification sticks to a few values for its purposes, then it can patch the schemas in its scrapers like I do in ours: https://github.com/opencivicdata/scrapers-ca/blob/master/patch.py

It makes no sense for a generic library to limit motion classification to such a restricted set.

For our Toronto vote scraper, we just store the raw classification - we don't care about normalizing it because we don't need to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I assumed we'd just add to that set as time goes on, let me think it over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'd hate to see everyone spell stuff differently for kicks, but I guess it's hard to nail down all the terms)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, I'm not normalizing motion classification, and I shouldn't have to. If I just add the raw Toronto values to python-opencivicdata-django, it's going to be a messy list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if these are settings and if it is blank then no validation is
performed (by dynamically checking for that in the schema nad not adding
the enum key if so)

On Tue, Feb 24, 2015 at 2:50 PM, James McKinney notifications@github.com
wrote:

In pupa/scrape/schemas/vote.py
#149 (comment):

@@ -7,7 +7,7 @@
"properties": {
'identifier': {"type": "string", "blank": True,},
'motion_text': {"type": "string" },

  •    'motion_classification': {"items": {"type": "string", "enum": common.VOTE_CLASSIFICATIONS},
    
  •    'motion_classification': {"items": {"type": "string"},
    

The think is, I'm not normalizing motion classification, and I shouldn't
have to. If I just add the raw Toronto values to
python-opencivicdata-django, it's going to be a messy list.


Reply to this email directly or view it on GitHub
https://github.com/opencivicdata/pupa/pull/149/files#r25285718.

James Turk
Labs Director | Sunlight Foundation http://www.sunlightfoundation.com/
202-558-8723 | @jamesturk https://twitter.com/jamesturk

[image: Sunlight Foundation] http://sunlightfoundation.com/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Euch, I just saw your comment after I merged, sorry @jamesturk

(but I think we agree, and I filed a bug #153 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work for me. If the solution can be applied for all enums, that'd be even better!

@paultag paultag merged commit 8731e79 into opencivicdata:master Feb 24, 2015
@jpmckinney jpmckinney deleted the voteclass branch February 24, 2015 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants