search

GMOD / Chado

the GMOD database schema
http://gmod.org/wiki/Chado
Artistic License 2.0
38 stars 25 forks source link

V1.3.3.010 - Adding a fact table for environmental data #108

Closed guignonv closed 1 year ago

guignonv commented 5 years ago

Adding a fact table for environmental data as described in #107.

scottcain commented 5 years ago

This commit/pull request isn't compatible with the flyway build system, so the pull request as currently formatted is destined to be rejected. Since it's the addition of a new table, this is a medium change, so it will require four reviews including at least one non-tripal reviewer. On the upside, since it is "just" adding a table, writing the flyway migration should be easy (written as someone who hasn't actually done it yet :-)

scottcain commented 5 years ago

@guignonv it would be great if you could rework this as a database patch in the "migrations" directory, naming it along the same lines as other files in there (so, something like "V1.3.3.009__add_environmental_fact_table.sql"). Also, putting "V1.3.3.009" in the title of the pull request would be helpful. Thanks!

guignonv commented 2 years ago

Following yesterday conversation on gather.town, I've merged Chado branch chado/1.4 into this PR (commit e5cfe8a1e43b4cfd0643bd2bd23055489f613ac2) to include a new migration file V1.3.3.010__add_environmental_fact_table.sql (commit e91dc32c7c4747092c23f164c81977919f58417f). Then, I've taken into account the modifications made by @scottcain (commit 8d3f75d50a148de64cde3c0ff1db050cedbf31c6) following @laceysanderson comment to add a "timecapturedend" column and included them both in the Natural Diversity module and the migration script.

So this PR should be merged to the chado/1.4 branch.

Did I forget anything?

scottcain commented 2 years ago

Yeah, the maintenance or not of the chado/modules/ directories is an annoying problem: I would like to keep them around, as they are an easy place to keep things organized by module, so if I want to look for something nd-specific, I know where to find it. The problem is that there will be guaranteed code drift.

scottcain commented 2 years ago

Not only that, but with the implementation of #79 , the modules directories have gone away!

spficklin commented 2 years ago

I was going to review this PR but it has a conflict as it has the chado/modules/natural_diversity/natural_diversity.sql file in it. From the conversation above it sounds like that file needs to be dropped. That's an easy enough fix and I'm happy to fix the merge conflict but I want to double-check that removing the file is the right thing to do.

laceysanderson commented 1 year ago

I just talked with @scottcain in Gather.town and he agrees @spficklin that the right approach here is to just remove the natural_diversity.sql file from this PR and then we can carry on with reviews as normal.

So I will remove the file right away :-)

laceysanderson commented 1 year ago

Closing this PR in favour of #137 which uses a branch from this repository for easier testing and resolves the merge conflict. All review should occur there.