I refactored how tables are wrapped, so various kinds of tables can use a few central functions, including declareCatalog and declareSortedCatalog (C++) and addCatalogMethods (Python).

I also tweaked the C++ formatting to match our standards or common usage, including changing
`using namespace lsst::afw::table;` to `namespace lsst { namespace afw { namespace table`
and `const std::string &` to `std::string const &`.

All the tests pass except testSourceMatch.py, which segfaults unless pickling is prevented, e.g. by adding a `return` at the beginning of `checkPickle`. Pickling will be fixed in DM-8415.

I am open to suggestions on how to better group the commits. I tried to group related changes but it feels a bit messy.

I think we should really squash most commits together into a single "Wrap afw::table with pybind11" commit on DM-6168. Only when things get changed in the library code itself (i.e. not the wrappers or tests thereof) should these commits be kept separate. Otherwise the final merge to master will introduce far too many pointless commits.

That makes sense. What about adding new library code that is only intended for pybind11 wrapping (as this ticket does)? I'm hoping it can all be part of the same commit, since it'll be messy to tease that out otherwise.

Yes, I would say so. In the end just having one commit per package for the wrapping makes most sense. Only in the case of afw, because it is so big, might it make sense to split it up per component namespace.

Having fewer commits also makes continuous rebasing on master way easier.

A had few minor comments on the ticket and a suggestion about the structure based on a conversation with Pim. In general I think it is a good idea to stick with the convention that there should be a pybind11 .cc file for each .h file in the include directory tree, but in this case I believe an exception was warranted and that it was a good change. It is much easier to understand where SimpleCatalog, BaseCatalog, etc are defined using your new structure.

I'm not convinced that switching to the namespace lsst { ... usage is correct in this case. My understanding is that there is a difference between using and namespace is that the current method adds the declare functions to the namespace, whereas the older method did not. I like how you added a _all_ definition to the python modules to keep the namespace clean but I feel like this does the opposite. Perhaps I'm misunderstanding what it's doing but I think for this reason and for the consistency of the pybind11 code it might be best to switch these back.

This also needs to be rebased with DM-6168. The commit history of the pull request should only show the commits that you made, not the past commits. It's possible that DMTN-026 needs to be updated since Pim realized the correct procedure to rebase properly. It should be

checkout tickets/DM-6168

git fetch

git reset --hard origin/tickets/DM-6168

git checkout tickets/DM-8264

git rebase --onto tickets/DM-6168 a003834~ tickets/DM-8264

where a003834 is the first commit you want to place on top of the rebased "master", DM-6168.

And make sure that it passes the tests in python 2 and 3 in Jenkins. It looks like you will need to include DM-7797 in your Jenkins build (there were some tests commented in that ticket that have not made their way to DM-6168 yet, which currently does not rebuild) or comment them yourself in this ticket.

DM-8264 is already rebased to DM-6168 (and I have done so several times during development) and I planned to do so again just before merging back to DM-6168, if necessary. Are you saying that I did this incorrectly? It looks right to me in SourceTree.

No, you did everything correctly. The PR just had the diff against the wrong base (master instead of DM-6168).

Pim Schellart [X] thank you for the explanation. Fred Moolekamp my apologies for making the PR with the wrong base.

Thank you for the helpful review.

I left the commented-out methods alone because I found it so useful when other methods I needed were already available but commented out. We will have to purge them at some point.

I also retained our standard usage of namespace lsst { namespace afw { namespace table and hope we will switch to that for all future pybind11 code in order to match all our other C++ code.

Other than that I made all requested changes.

Tested with Jenkins using Python 2 and Python 3 and merged to DM-6168.