DemocracyClub / uk-election-timetables

Encapsulates timetable legislation for elections run in the United Kingdom and its devolved administrations. 🗳🕰
MIT License
3 stars 0 forks source link

Decompose `StatementPublishDate` into multiple class, one per election type #1

Closed mrwilson closed 3 years ago

mrwilson commented 3 years ago

This is a proof-of-concept for how the underlying model in the library could change to adapt to the broader scope.

When the library was solely concerned with SoPN publish dates, there was a single component StatementPublishDate with a function for each election type.

Now there are multiple interesting dates for each election type.

One way to handle this is to make the election the top-level object rather than the type of date.

This PR is an example of how we'd do that for Senedd Cymru, the Welsh Parliament. If merged, we would roll this out to all election types, which would simplify adding new dates and further election-specific logic into their own classes.

symroe commented 3 years ago

This is great, thanks for taking a look!

I like the approach of making the election the top level with the other dates falling out of it.

A couple of thoughts after reading the diff - I've not left line comments as I'm not sure they're very useful at the moment.

  1. I think it would be nice to pass the date when instantiating the Election, rather than passing it for each date type - did you have a reason for adding it to each date type?
  2. It would be nice to be able to pass an election ID in and have the library work out what subclass of Election to use - have you thought about this case? Without it, I can see client application having to perform this logic, so I think it's worth including in the library somewhere. (e.g, include the for_id concept we have at the moment)
  3. Feature request - when we have n date types, it might be nice to be able to get a list of them all (for a given election type), ordered by the date. Imagine trying to make a timetable of all the dates for a given election as a use case.
  4. Python style point, it would be nice to use abc with abstractmethod on the base Election class. This will force subclasses to implement all required methods
  5. I really like the legislation URL in the docstring - really nice to have the "business logic" next to the code ;) I wonder if we can find a way to expose this in python somehow? Not sure why, but it seems cool to be able to form citation links for a given statement about a date (total stretch goal, of course!)
mrwilson commented 3 years ago

I think this big refactoring is done now. The API has changed quite a bit.

from uk_election_timetables.election_ids import from_election_id

election = from_election_id("local.2019-02-21", country=Country.ENGLAND)

print(election.sopn_publish_date()) # date(2019, 1, 28)

A few things:

I'm pretty happy with this API change. Adding further dates or a .timetable() method on Election will be relatively easy, I think.

Next few steps:

It feels like this should be a major version bump up to 2.0.0 because of the total API incompatibility with sopn_publish_date

What do you think, @symroe?

symroe commented 3 years ago

This is really good!

After checking the code out I realised I can't just run pytest like I'd expect to, so I've pushed a commit to fix that. I've also ignored __pycache__ files.

I think the last change I'd like to see is moving sopn_publish_date (etc) to a property rather than a method. I don't think doing election.sopn_publish_date() makes sense when we could have election.sopn_publish_date

mrwilson commented 3 years ago

This looks ready to merge - sopn_publish_date is now a property

symroe commented 3 years ago

Great stuff!