-
Notifications
You must be signed in to change notification settings - Fork 43
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 readnoise monitor to use django database models #1482
Update readnoise monitor to use django database models #1482
Conversation
Hello @york-stsci, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2024-02-14 21:36:58 UTC |
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.
This looks ok to me, as someone who is not a Django model expert. And I'm assuming there's no issue with defining the models for all of the monitors in this PR, even though the monitors themselves are not yet switching over to use them.
I do think the Django environment variable setting and call to setup() probably need to be in an if clause so that they're not run on RTD or GA.
The models were actually all defined in a previous (and merged) PR. This PR touches all of those files because, when I found out that I needed to change the (incorrect) data types that django had assigned to array fields, I just did it for all the monitors rather than stop with readnoise. |
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.
This looks good to me with only a mispelling in the doc.
Aside from readnoise.py, I did not dive deep into the monitor_models as they will be changing.
Especially excellent work with the documentation update, thanks for taking the time to do that!
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.
Hey @york-stsci just a few questions but this is huge improvement, thank you for submitting this!
This PR is a test of updating one of the monitors (readnoise) to use the django database model, both for insertion and retrieval. It also adds general documentation describing the process.