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

Do not pass popolo data to wikidata page #14849

Merged
merged 2 commits into from
Aug 23, 2016

Conversation

ondenman
Copy link
Contributor

@ondenman ondenman commented Aug 22, 2016

HouseWikidata now passes two arrays. One is a list of people with wikidata, the other is a list of people without.

Closes #14839

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 12:27 Inactive
arr.push( { name: person.name,
wikidata_identifier: wikidata_identifier,
wikidata_url: wikidata_url
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to collapse this down to our own data structure here — it's fine just to pass in a list of EveryPolitician::Person objects

@ondenman ondenman force-pushed the 14839-do-not-pass-popolo-data-to-wikidata-page branch from bf60429 to 1426853 Compare August 22, 2016 15:29
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 15:29 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 15:34 Inactive

def people_with_and_without_wikidata
@with_and_without_array ||= popolo.persons.partition(&:wikidata)
{ with: @with_and_without_array[0], without: @with_and_without_array[1] }
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems a little needless to convert this to a hash here, rather than just calling people_with_and_without_wikidata.first or .last in the public methods.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:06 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:14 Inactive
@@ -1,4 +1,5 @@
# frozen_string_literal: true
require_relative '../popolo_helper'
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you need to add this? I can't see anything that should require it, and removing it doesn't cause any obvious failure.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:18 Inactive
@ondenman ondenman force-pushed the 14839-do-not-pass-popolo-data-to-wikidata-page branch from 960e4ea to 1c473f8 Compare August 22, 2016 16:24
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:24 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:26 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:27 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 22, 2016 16:55 Inactive
andrea = '0eedf2c9-01ea-44f4-bc6e-e5e4bf6d2add'
subject.popolo.persons.find_by(id: andrea).wikidata.must_equal 'Q493950'
it 'should pass a list of people with wikidata' do
res = subject.people_with_wikidata.select { |p| p[:name] == 'Cornelia Ecker' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Straggling [:name] rather than .name. This might be clearer, though, as

subject.people_with_wikidata.map(&:name).must_include 'Cornelia Ecker'

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 23, 2016 08:58 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 23, 2016 09:04 Inactive
@ondenman ondenman changed the title WIP 14839 do not pass popolo data to wikidata page 14839 do not pass popolo data to wikidata page Aug 23, 2016
@tmtmtmtm
Copy link
Contributor

Yay! 👍

There are a lot of VCR cassettes here that I don't think are needed, but I want to squash these commits down a bit first anyway, so I'll clean those out whilst I'm doing that.

@tmtmtmtm tmtmtmtm force-pushed the 14839-do-not-pass-popolo-data-to-wikidata-page branch from 223e19f to 9448aec Compare August 23, 2016 11:23
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-14849 August 23, 2016 11:23 Inactive
@tmtmtmtm tmtmtmtm force-pushed the 14839-do-not-pass-popolo-data-to-wikidata-page branch from 9448aec to 8f56c64 Compare August 23, 2016 11:35
@ondenman ondenman changed the title 14839 do not pass popolo data to wikidata page Do not pass popolo data to wikidata page Aug 23, 2016
@tmtmtmtm tmtmtmtm merged commit 6303014 into master Aug 23, 2016
@tmtmtmtm tmtmtmtm deleted the 14839-do-not-pass-popolo-data-to-wikidata-page branch October 1, 2016 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants