search

openplans / openblock

OpenBlock is a web application and RESTful service that allows users to browse and search their local area for "hyper-local news
61 stars 26 forks source link

refactor Blocks, Streets, Location, etc. to share a common API #70

Open slinkp opened 12 years ago

slinkp commented 12 years ago

There's a fair amount of risk involved in this ticket, and all it would accomplish is some cleanup. But it's driving me crazy, so:

The location-aware models in ebpub.streets.models and ebpub.db.models do not share a common base class. In some cases we can't even use duck-typing because most location-aware objects have a .location geometry field, but Block has instead .geom.

Also, it's confusing that we use the name .location to refer to a geometry and not a Location, except NewsItemLocation.location is a Location :-p

Proposal:

  1. Change all geometry column names to .the_geom instead of .location or .geom. This appears to be a very widespread naming convention in the GIS world, which we should not buck for no reason; and it would disambiguate whether the column refers to a geometry or a Location object. Properties can be added for a modicum backward compatibility (although this would break lots of queries that will have to be rewritten).
  2. Make an abstract base class for all location-aware classes. Or possibly use multi-table inheritance, which adds some joins for looking up the extra fields, but means we can do one search to find everything that eg. overlaps some geometry. Common APIs could go here. Not sure if we even need this; duck typing would suffice. (Note that .the_geom can't go in the superclass because it is sometimes a Point and sometimes a LineString, and children can't override an ABC in django. But a fair amount of stuff could go here like .pretty_name)
  3. Anytime there's a .location attribute, it should refer to a Location instance, or at least an instance of something that has .the_geom.
  4. Gradually remove code that checks the type of location-aware object (eg. there's a lot of things that test whether you have a Block) and try to use polymorphism instead.

More modest variation of this proposal:

  1. change Block.geom to Block.location. Just live with the choice of name.
  2. as above.
slinkp commented 12 years ago

Ticket imported from Trac: http://developer.openblockproject.org/ticket/70 Reported by: slinkp