briot / geneapro

Genealogical software based on the GenTech data model
http://briot.github.com/geneapro/
GNU General Public License v2.0
33 stars 8 forks source link

Consolidated `add_ancestors()` and `add_descendants`. #58

Closed changeling closed 5 years ago

changeling commented 5 years ago

Consolidated add_ancestors() and add_descendants into single function add_folks(). Updated pedigree.py, quilts.py, stats.py and queries.py to add_folks() functionality.

changeling commented 5 years ago

Not sure if you want this, but here's a consolidation of the add_ancestors and add_descendants into one function.

briot commented 5 years ago

Not sure if you want this, but here's a consolidation of the add_ancestors and add_descendants into one function.

I do want it. I had planned to do it at some point too.

I would use an enum type instead of strings for the type of relationship, but otherwise looks good to me.

I need to review a bit more, but perhaps the ‘key’ can be hard-coded to a common value too, so that you do not have to make it depend on the type of relationship. And then if you pass directly get_ancestors or get_descendants, these can replace relationship altogether and avoid the string comparison. Not sure whether that works in practice, I did not have time to check yet.

Thanks ! .

changeling commented 5 years ago

I would use an enum type instead of strings for the type of relationship, but otherwise looks good to me. I need to review a bit more, but perhaps the ‘key’ can be hard-coded to a common value too, so that you do not have to make it depend on the type of relationship. And then if you pass directly get_ancestors or get_descendants, these can replace relationship altogether and avoid the string comparison.

Great! And yes, I intend to change to something else, probably the enum type approach. This is just sort of my process, and perhaps I shouldn't be submitting. Just wanted you to confirm it was welcome, really, and offer it for review. I'm still playing with it. I've gone a bit farther now, as you'll see in the last commit. And, as I said, the string/if-then/etc approach is basically me thinking out loud, as it were. Not the intended final.

briot commented 5 years ago

I just commit significant patches that create conflicts here. Mostly this is because queries.py was moved to a new subdirectory sql/sqlsets.py. I had been waiting to check whether you could submit a revised PR first, but after a few days I feel better if I can push my changes... Sorry for the extra work, let me know if you need help

changeling commented 5 years ago

No worries! I've been swamped and unable to get to it effectively before now. I'll gladly pay the price for my delay. :D

changeling commented 5 years ago

Closing this PR in preference for a cleaner version incorporating changes to master, including PR https://github.com/briot/geneapro/pull/60.