everypolitician / everypolitician-pull_request

Generate a summary-of-changes comment on EveryPolitician data pull requests
MIT License
0 stars 0 forks source link

Handle changing fields in Memberships #12

Open tmtmtmtm opened 7 years ago

tmtmtmtm commented 7 years ago

Currently if one field in a Membership record changes (e.g. an end date is added), we report that one Membership (with no end date) has been removed, and one Membership (with the end date) has been added. (See, for example, https://github.com/everypolitician/everypolitician-data/pull/20111)

It would be nicer if we could report things like that as a change, rather than a removal+addition.

chrismytton commented 7 years ago

This should be fairly straightforward. The Everypolitician::Entity class in everypolitician-popolo defines a == method, so we should be able to use that to identify objects with the same id. Then we can compare the .document of memberships with the same ids to see if they've changed.

tmtmtmtm commented 7 years ago

Ah, but don't forget that Memberships don't have an ID, and are thus compared by whether all the fields are the same or not… (https://github.com/everypolitician/everypolitician-popolo/blob/40dc460d8340df9ae78469b755ce73b4fd074ce3/lib/everypolitician/popolo/membership.rb#L68-L70)

chrismytton commented 7 years ago

@tmtmtmtm Oh of course, and that's why it doesn't Just Work at the moment like it does for other Entity subclasses. So yeah, this is a bit more complex than I initially thought.

chrismytton commented 7 years ago

Simple first version of this can just report when the URL of the membership source changes.

tmtmtmtm commented 7 years ago

@chrismytton I don't think that's worth reporting on. I'd simply not have the summariser report on a changed source at all.

chrismytton commented 7 years ago

Had a look at this but it's trickier than I thought.

Currently we're only comparing things at the Popolo level. In order to check for a membership source URL changing we'd need to check if the instructions.json file had changed and if so fetch that and then go through it and compare the sources to the previous version.

While this is certainly possible it's probably more work than just doing the comparison of the memberships at the Popolo level to see if they've changed.

I think it's probably best to just do the work to report on memberships themselves changing, rather than jumping through hoops to get access to instructions.json in order to compare source URLs.

tmtmtmtm commented 7 years ago

@chrismytton I'm not following this at all. The source URL for a Membership is in the Popolo, so I'm not sure where instructions.json comes in. This is about the summariser not reporting on changes like the following (from https://github.com/everypolitician/everypolitician-data/pull/24902/files), where the only thing in the Membership record that has changed is the source:

screen shot 2017-03-09 at 14 46 35

Currently those show up in the summariser comment, which is mostly just noise, when the only memberships have changed in a significant-enough way to report are those for the Lavulavus:

screen shot 2017-03-09 at 14 48 34

chrismytton commented 7 years ago

Not quite sure what I was thinking there, bit of a brain fail! Have opened a pull request to ignore changes to the sources array over in #22.