Impose restrictions on name field #291
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
- Coverage 72.24% 71.86% -0.38%
==========================================
Files 26 26
Lines 3210 3220 +10
==========================================
- Hits 2319 2314 -5
- Misses 891 906 +15 ☔ View full report in Codecov by Sentry. |
| ) | ||
| if result.status_code != 200: |
There was a problem hiding this comment.
I don't think this has to do with name field restrictions. :)
There was a problem hiding this comment.
Yeah, we were getting a test failure on two tests because of this
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| import pytest | |||
| from astropy.coordinates import SkyCoord | |||
| from requests import ConnectionError | |||
| from requests import ConnectionError, ReadTimeout | |||
There was a problem hiding this comment.
I'm guessing this has to do with allowing timeout of TESS access attempt, not the name field restrictions.
There was a problem hiding this comment.
Correct...I wanted to make the lights green
| if not value.strip(): | ||
| raise ValueError("name must not be empty or contain only whitespace.") | ||
|
|
||
| if not re.search(r"^\S$|^\S.*\S$", value): |
There was a problem hiding this comment.
So you are matching a SINGLE non-whitespace character as the string name or any string that starts and ends with non-whitespace characters? I guess this makes sense, as I think of ways to avoid leading or trialing spaces... you have to allow for the possibility of a single character or multiple characters. It does look odd, but the logic is sound.
There was a problem hiding this comment.
Yes, that is the intent. I thought about just stripping out whitespace silently but it seemed but to flag it instead.
| @@ -221,9 +249,10 @@ class Camera(BaseModelWithTableRep): | |||
| model_config = MODEL_DEFAULT_CONFIGURATION | |||
|
|
|||
| name: Annotated[ | |||
| str, | |||
| NonEmptyStr, | |||
There was a problem hiding this comment.
Took me a bit, but I see this is now using that NonEmptyStr validator you set up above...
I realized while working on the form for entering
Cameraand other named objects that it allowed a bunch of bad names, like""or" ". Since leading or trailing whitespace in a name also seemed like a bad idea, this PR adds (and tests) this constraint on the name field:A quick review on this would be helpful...