Add election and legislative period classes for events

[struan]

This adds terms/legislativeperiods and elections methods to popolo that return specific classes for those event types. It also updates Events to generate those classes as part of the Events collection.

Fixes #67

[tmtmtmtm]

I'm assuming this means that legislative_periods has now reversed order? That's not necessarily a bad thing, but it definitely needs to be explicit in the CHANGELOG.

[struan]

Ah no, this should probably have squashed down as it's just reverting a change in e81a50f.

[tmtmtmtm]

We've only just removed one giant case statement for this sort of thing. Bringing back another one doesn't seem ideal. We can't quite do the same declarative in-place approach as then, but making this data driven seems like it might be slightly better.

[struan]

So, this is the problem I was trying to solve with all the meta-programming in #75. I can't see how to make this work without some way of getting a list of all the possible subclasses to match against.

We could improve it slightly by pushing the match down into the subclass but we'd still need a case statement. A further improvement might be to have a list of subclasses and then looping over those doing the matching based on a property in the class which reduces the code a bit:

      def self.new(doc, *args)
        subclasses = [Election, LegislativePeriod]
        klass = subclasses.select { |s| s.classification == doc[:classification] }.first || Event
        klass.new(doc, *args)
      end

which needs a bit of tidying but does work with the addition of classification to Entity and the relevant calls in Election and LegislativePeriod.

I did have a look at how to push things up from the subclasses into the parent in a add_match_class Election sort of way but it was looking like being a bit awful and also feels wrong.

[tmtmtmtm]

I was thinking of something as simple as:

      def self.new(doc, *args)
        classifications = {
          'general election' => Election,
          'legislative period' => LegislativePeriod,
        }
        classifications.fetch(doc[:classification], Event).new(doc, *args)
      end

(Though with also moving that Hash definition out of new — inlined here just as proof of concept)

As with your approach the primary goal is simply to rid of all that duplicated logic in the case statement (and indeed the case itself, which is pretty much always a warning flag in anything OO based), and move the thing that varies to data instead, which should be easier to maintain, alter etc.

But your suggestion seems a little better. Ideally we want to follow Open/Closed here — adding a new subclass shouldn't require changing a superclass too (both for maintainability, and because it's very bad practice for a superclass know anything about its children).

[tmtmtmtm]

Is there a better name for this, based on what it is, rather than what it does?

(Also, what's the path to adding more of these, e.g. for having separate Party and Legislature subclasses of Organization? Do we extend this? Or add a parallel version somewhere?)

[tmtmtmtm]

I'm still not entirely sure whether we're heading in the right direction with this. Some of this, I suspect, is because the Popolo we're dealing with is generally still quite slim, compared to what we're likely to have in the (hopefully not-too-distant) future, as we start to add many more types of information.

But I'm having some difficulty trying to reason about how some things in the future will need to work. (NB this isn't about getting everything perfect now — but this is a library that external people are using, and which we actively want lots of people to use, so I'm particularly cautious that we don't want to paint ourselves into too many nasty corners with approaches that people come to rely on but turn out to have been the wrong design)

One realisation I had whilst thinking about this again this morning is that the Organization example I've been using will get even more complicated when we have something like Committee memberships, as that will have an impact on both Organization(s) and Membership(s). I suspect it's going to be worth mocking up an example Popolo file with those, and making sure our current approach is going to make it not just possible, but ideally fairly trivial, to add a new Organization type like that (ideally entirely declaratively), and have everything Do The Right Thing™.

(That we're also actively trying to change various other things about how the class hierarchy works at the same time doesn't help (#88) — I'm starting to have some difficulty keeping the various changes in my head at the same time. So I also suspect that we should try to close #54 first, to avoid any more odd interactions between these. @struan I think you were already looking at that one at some point already? Might be useful if you can help @ondenman finish that one off before coming back to this one.)

[tmtmtmtm]

.select { }.first is generally simpler as .find { }

[tmtmtmtm]

if Events are already pre-sorted in popolo.rb, do we need to also sort here (and in legislative_periods below)?

[struan]

Closed in favour of #109