-
Notifications
You must be signed in to change notification settings - Fork 20
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
Faker #46
Faker #46
Conversation
This reverts commit c397117.
Signed-off-by: NanoNero <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @NNerurkar 👍🏽
My general concern is that this is a python package and the rest of the program is written in Java. I think it's okay for now, but ideally this would be a java program that can be run as part of maven script to instantiate the project. @siranjujugi looking for your input here. I think it's okay for now as we don't have another way to generate realistic data. We might eventually want to do the same in a Java/mvn way.
- Code observations:
- I see the accountID and the apiKEY are inside the code. Can we extract them to .env file. Or maybe you can reuse
https://github.com/Call-for-Code-for-Racial-Justice/Open-Sentencing-Aggregator/blob/main/src/main/liberty/config/server.sample.env
- I see time.sleep statements. I am wondering if you are doing that because the code is ansynchronous? If so, we should use async/await or something similar instead of waiting for a certain time. It seems very brittle to me.
- Can you please add some documentation around how to use this code to generate cases, etc? This can go back in the main README.md file for aggregator.
- what is the purpose of this code?
- example on how to run the code. What parameters are needed.
- example run and output
- Additionally, can you please add some unit tests? Something I can think of testing:
- check for mandatory parameters if any
- check for error conditions
- passing in null or None vlaues
- passing in wrong data datatypes
@NNerurkar pls also fix the DCO error by following the |
Signed-off-by: NanoNero <[email protected]>
This reverts commit c397117. Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Hi @NNerurkar, something I noticed in the demo is that the script is adding documents to databases with different names than is supported by the aggregator. The "Create and deploy Aggregator application" section of the README.md mentions "two Cloudant databases are required: outcarcerate-attorney and outcarcerate-client". I actually think having more databases is a superior data model than is currently implemented. However, the aggregator now adds cases, charges, and sentences as part of the attorney document. Changing the data model used by the aggregator would require a fair amount of work and be a separate ticket. Running a few queries via the swagger UI after adding documents with faker should help to identify any discreprencies with the data model. |
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
Signed-off-by: NanoNero <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making all the changes @NanoNero
Contributes to #44
What did you do?
Add a script to populate a Cloudant database with randomly generated documents.
The script adds documents for 5 classes - attorney, client, case, charge, sentence.
Why did you do it?
How have you tested it?
Were docs updated if needed?
Type of change
Checklist: