-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update NormedPerson to serialization logic similar to NormedPublication #137
Comments
@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 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) |
@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 |
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. |
This commit moves load from CSV logic closer to the data structure interface. And while it doesn't check off all of the TODO items for #137, it helps tidy up the code base. I'm going to be reviewing the details of those checkbox items, as I'm unclear the purpose (e.g. For example, writing the data structure to CSV)
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 Is this something you have on your machine or in another branch? |
Expanding NormedPerson interface to class
This commit moves load from CSV logic closer to the data structure interface. And while it doesn't check off all of the TODO items for #137, it helps tidy up the code base. I'm going to be reviewing the details of those checkbox items, as I'm unclear the purpose (e.g. For example, writing the data structure to CSV)
Expanding NormedPerson interface to class
Similar to NormedPublication, do the following
The text was updated successfully, but these errors were encountered: