Skip to content
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

Open
4 of 9 tasks
rickjohnson opened this issue Feb 8, 2021 · 4 comments
Open
4 of 9 tasks

Comments

@rickjohnson
Copy link
Contributor

rickjohnson commented Feb 8, 2021

Similar to NormedPublication, do the following

  • Create JSON mapping file for person similar to normedPublicationObjectToCSVMap.json
  • Convert loadPerson to static method in NormedPerson.ts and rename to loadFromCSV so that it would be called in the form NormedPerson.loadFromCSV with returning NormedPerson[]. Use JSON mapping file in load
  • Create writeToCSV static method in NormedPerson.ts takes NormedPerson[] and filepath as parameters, using JSON mapping
  • Convert NormedPerson from and a Typescript interface to a Typescript class similar to NormedPublication
  • Convert usages of loadPerson in other scripts to use new method
  • Move queryNormalizedPeople methods into NormedPerson.ts (perhaps rename to loadFromDB ?)
  • Audit the db attribute names in DB and NormedPerson to see if they can be normalized together
  • For unit tests that use loadPerson convert to use new methods and json mapping
  • Update unit tests accordinly
@jeremyf jeremyf self-assigned this Feb 9, 2021
@jeremyf
Copy link
Contributor

jeremyf commented Feb 9, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor

jeremyf commented Feb 9, 2021

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 added a commit that referenced this issue Feb 10, 2021
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)
@jeremyf
Copy link
Contributor

jeremyf commented Feb 10, 2021

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?

rickjohnson added a commit that referenced this issue Feb 10, 2021
@jeremyf jeremyf removed their assignment Nov 23, 2021
rmakhija94 pushed a commit that referenced this issue Apr 18, 2023
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)
rmakhija94 pushed a commit that referenced this issue Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants