Lucy-Family-Institute / pace-admin

Publication Activity Collection Environment - Admin Tools and DB
Apache License 2.0
1 stars 4 forks source link

Update NormedPerson to serialization logic similar to NormedPublication #137

Open rickjohnson opened 3 years ago

rickjohnson commented 3 years ago

Similar to NormedPublication, do the following

jeremyf commented 3 years ago

@rickjohnson I have a question about the purpose of this work; In the work breakdown, it looks like I'm gathering up methods that are presently scattered throughout the code base. The goal being to put those methods in a common class.

What confuses me is the purpose of the "Create JSON mapping file for person similar to normedPublicationObjectToCSVMap.json" task. In the normedPublicationObjectToCSVMap.json this looks like the JSON keys are the attribute names for the interface/ingest class. And the values are the CSV column name.

I guess I'm struggling to see the necessity of this mapping file. Do we already have a case where this process would have two different structured CSVs that would load the normedPerson? It seems that this is an unnecessary optimization at this point (e.g. we're encapsulating the read/write from CSV). In reading the code, it's create a bit of indirection to comprehension. (e.g. to know what's happening, I need three files open: CSV, JSON Map, and Code)

rickjohnson commented 3 years ago

@jeremyf you are right it may not be necessary. I originally created it for pubs thinking it might be good to have these mappings be configurable and originally had it in the config dir. I then moved it 'closer' to the normedPublication code after realizing a change to it might be a code-breaking change. So... an improvement could be to fold that into the normedPublication class, and have something similar embedded in the normedPerson class

jeremyf commented 3 years ago

That's kind of been my framing; I'll see where I go with the implementation.

It's been difficult scheduling time because I have a meeting, then an hour then a meeting. I'm going to block out time later this week.

jeremyf commented 3 years ago

I've updated the checkboxes to reflect PR#141.

However, I'm at a loss with several next steps regarding this ticket. In particular I am not finding any queryNormalizedPeople in the code-base; So I'm hard-pressed to go down that path.

Is this something you have on your machine or in another branch?