tripal/t4d8

Upgrading Importer use of old Chado Prepare to the new Task system

risharde opened this issue · 6 comments

Hi @laceysanderson, I wanted to try a test recompile of your importerReviewed branch and my additional changes. I began by creating a test branch from 9.x-4.x and merged the importerReviewed branch. There was a specific conflict regarding the preparer code in tripal_chado/src/Task/ChadoPreparer.php of which I allowed both changes to get the function prepare() back in.

During the test to prepare which is initiated by Admin -> Tripal -> Chado -> Prepare chado, I think the prepare function isn't called.
So 2 questions:

  1. Not too sure where to call the prepare function from with the changes (didn't spend a lot of time studying the new code)
  2. Or is there a better way to integrate the prepare() into the new task system

Please advise, since I am currently getting errors trying to do a test of running the Sequence Ontology import.

Here is the test branch pushed if you could have a quick look: https://github.com/tripal/t4d8/tree/tv4g4-4-tripal_importer_make_preparer_compatible

I'm also just tagging @spficklin to keep him up to date on what I'm working on.

Okay I just saw there's a performTask() function so I'm going to 'stupidly' try to just add the prepare function in there until you guys clarify the best way to tackle this one

Alright so I renamed the branch after adding the prepare function since this code would be needed to make the tripal importers work and should thus be merged into reviewedBranch as well. https://github.com/tripal/t4d8/tree/tv4g4-4-tripal_importer_make_preparer_compatible

Hi @risharde,

I warned you about this change. The new prepare still does the same thing it is just wrapped within the new Tripal BioDB Task API. It is called the same way the old one was and is integrated completely. I suggest comparing the old and new class to see the difference but essentially the new performTask() method maps to the old prepare() method with the addition of progress reporting. The methods below that are literally a straight move from the old class so your additions should go there and be completely integrated with the existing methods. The performTask() method should just call the other methods were most the code lives.

@laceysanderson I need to double check the code, need to understand what you mean by map. I can't recall seeing a prepare function in the new one so I merged it in and then called the prepare function from performTask. So I suspect that what you want me to do is remove the prepare function but the function calls within there, put that into the performTask ? Just making sure before I do. It's the wording that's getting me, sorry

So I suspect that what you want me to do is remove the prepare function but the function calls within there, put that into the performTask ?

Yes, without duplicating any of what's already in performTask() of course :-) Also, make sure that most of your code is in well-named methods and that performTask only calls those methods as done so far.

This functionality was submitted by Rish through a PR and thus this is resolved.