-
Notifications
You must be signed in to change notification settings - Fork 9
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
Input validation to handle 502 Errors #87
Input validation to handle 502 Errors #87
Conversation
I went in and made a small change to the |
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.
Had a few comments, mostly moving to some shared pieces that makes your code cleaner :) Hopefully this can remove that ruff ignore statement too, no big deal if it doesn't though
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.
There is one comment that needs updating but other than that, looks good! Thanks for making those changes!
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.
Awesome work here, this looks great! And great work on making that parameterized test. That is some advanced level unit testing 🔥
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.
Looks great, just some very minor stylistic comments that you can take or leave.
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
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.
Thanks for making those minor updates, I think it reads nicely now. I found one minor thing in the error string example that should be updated too, otherwise looks great!
imap_data_access/io.py
Outdated
and not file_validation.ScienceFilePath.is_valid_repointing(repointing) | ||
): | ||
raise ValueError( | ||
"Not a valid repointing, use format repointing<num>," |
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.
Remove the ing
and make it explicit how many numbers there has to be. (Update the actual code as well, I just noticed this in the test here)
"Not a valid repointing, use format repointing<num>," | |
"Not a valid repointing, use format repointXXXXX," |
f313518
into
IMAP-Science-Operations-Center:main
Adding input validation for some parameters to avoid 502 errors
Overview
Input validation code for all input parameters. This should avoid most if not all 502 errors triggered when incorrect data is passed to the query and it crashes.
Also introduced a test to iterate over bad input values and check that the correct valueError message is returned.
Closes #55
New Dependencies
New Files
Deleted Files
Updated Files
Testing