databaker icon indicating copy to clipboard operation
databaker copied to clipboard

Move the bag overrides into fork of xypath

Open goatchurchprime opened this issue 7 years ago • 1 comments

At line 182 of overrides.py # === Bag Overrides ======================================= is a set of functions that are actually member functions of xypath.Bag, where bag===self

Same for some of the xypath.Table functions, like xypath.Table.excel_ref

(But leave any of the functions that refer to dimension, as these don't belong here at all!)

Can they be moved to a fork of xypath? Moving stuff between independent repos is rather awkward, which is probably why it got implemented this way.

goatchurchprime avatar Oct 03 '16 10:10 goatchurchprime

I agree the code could benefit from being better organised.

In this case, the "bag overrides" actually just add methods to xypath.Bag rather than overriding existing ones. There are other changes in there, e.g. xypath.xypath.Table.from_messy_ which do monkey patch existing methods.

If the changes are only useful for Databaker, then Databaker feels like the best place to put them to me. The monkey patching probably comes under this. If some of the Bag additions are potentially generally useful, rather than just needed for Databaker, we could move those into xypath.

I also note some of the code in there requires Databaker, whereas xypath currently doesn't have that as a requirement.

Options:

  • move code into xypath itself. I'm against moving these to a fork of xypath since that's something else to care for, and will require porting in fixes from the parent version every time it's updated. Also means there's possible confusion over people having the wrong version installed for Databaker.
  • move code into actual xypath, not a fork (possible clutter there, because it's not code needed for xypath and some are both ONS specific and require databaker, e.g. append_dimensions, also not easily possible to move everything in there as some code changes xypath's behaviour);
  • move code into a separate repo/package "xypath-databaker-overrides" or similar (adds another separate thing to install);
  • organise better into separate modules/subpackages within Databaker, so the xypath changes are separate from the messytables changes, and maybe separate out overrides from additional functionality.

Tidying it up within Databaker seems best to me, but I'm open to arguments to do something else.

StevenMaude avatar Oct 06 '16 10:10 StevenMaude