John Swinbank, if you're got a bit of time for reviewing while enjoying Aspen (if not, let me know, and I'll reassign), here's some afw::table refactoring for you to enjoy. More info is available on the GitHub PR:

https://github.com/lsst/afw/pull/26

Note that it's a diff relative to tickets/DM-2533, not master, because I'm trying to split up the reviews of all the DM-1766 subtasks to make them more digestible.

I'm happy to look at this, but I can't promise I'll get it done this week. If you're keen to have it merged before the (nominal) end of this sprint, you might need to ask somebody else – otherwise I'll get to it within a few days.

That's fine. I've got a ton of reviews out in parallel that have to all be finished before I'll be comfortable merging any of them, with some coding yet to do on my part, so I doubt you'll be the blocker in the end.

I enjoyed reading this code & found the exercise quite informative. I have added a number of comments to the pull request, but these are in general quite minor: most of them are just suggestions for where the documentation could be made a little more helpful to the reader. I've not bothered commenting on the whitespace issue that Tim brought up, but I do think splitting that into a separate commit would be helpful.

The only more substantive concern I have here is regarding testing, or, rather, lack thereof. The high level functionality is already covered by the existing tests, so that's fine. However, those tests do cover a lot of ground at once, and I can't help but wonder if lower level tests, directed at simpler units of code (like FitsSchemaInputMapper) wouldn't be helpful to provide some more fine-grained information when things break. That said, I realise that much of this functionality isn't exposed to Python, and that we prefer to do all our testing in Python code as far as is possible – it may be that testing at this level is impractical or not worth the effort, but I think it's worth considering.

One area where I'm concerned that the high-level test coverage may not be adequate is in the backwards-compatibility code. For example, SourceFitsReader switches between OldSourceFootprintReader and SourceFootprintReader depending on the input file. I think the test cases rely on creating some data, persisting it, and then demonstrating that it can be read correctly: presumably that will only ever test one of the possible code paths here.

The backwards compatibility test coverage is actually dealt with later in DM-1766, on DM-2536, where I've re-enabled/added two tests that operate on git-committed files that were generated with an older version of the pipeline - I didn't do it on this particular issue simply because I really needed all of the backwards-compatibility code to be in place to test any of it.

As for the more general question of how to test intermediate-level interfaces, that's a concern bigger than just this issue. I think the right solution is to still have some tests in C++, as I don't think it's generally good for our user-visible Python interfaces to have additional Python interfaces that are only used for testing. I'll take a look at FitsSchemaInputMapper and see if there's anything I think is worth testing directly, or alternatively, if there's anything that should just be removed since it's not used right now - I've certainly been guilty of adding useless methods in the past because I imagined they'd be useful someday in the nebulous future.

Not yet merged, but all review comments have now been addressed on DM-1766, where the final merge-to-master prep will occur.