- Anything in the code that would affect the integrity of the data and interpretation of the results
- Making it easier to extend reporting to more teams and to maintain reporting
- General best practices
Description
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | • Awjrichards | T108645 Get code review for Phab burnup reporting scripts (phlogiston) | |||
Resolved | • JAufrecht | T110599 Review SQL/Python for Phlogiston |
Event Timeline
I reviewed the python and gave Joel a bunch of notes. tl;dr was that everything basically looked good, with one potential big speedup (or not), and some ideas for making the code easier to understand.
Any objection to my moving the feedback from a google doc to a new Phab task (so I don't lose it/can treat it as backlog?)
My SQL skills are "intermediate", so I don't feel capable of doing a thorough review. I will say that the code is well-commented, and all the parts that I did understand (more than half) made perfect sense.
Personally, I would like to see most if not all of the code moved into python. The calls to export tables as CSV would (mostly?) be one-liners, and the munging could be there as well. In some cases, the python would be clearer, and in other cases it would be less clear, but overall I would call it a net win to no longer have raw SQL scripts as part of the process.
I think my part is done. Feel free to get back to me with questions or to request further review of specific pieces.
Done as soon as Joel stores Kevin's email feedback in a permanent, public location. Actual work to address anything is new tasks.