-
Notifications
You must be signed in to change notification settings - Fork 12
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
Impose restrictions on name field #291
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct...I wanted to make the lights green |
||
|
||
from stellarphot.io.tess import TessSubmission, TessTargetFile | ||
|
||
|
@@ -85,7 +85,7 @@ def test_target_file(): | |
|
||
try: | ||
tess_target = TessTargetFile(tic_742648307, magnitude=12, depth=10) | ||
except ConnectionError: | ||
except (ConnectionError, ReadTimeout): | ||
server_down = True | ||
tess_target = None # Assure tess_target is defined so that we can delete it | ||
except ValueError: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
# Objects that contains the user settings for the program. | ||
|
||
import re | ||
from pathlib import Path | ||
from typing import Annotated, Any, Literal | ||
|
||
|
@@ -9,6 +10,7 @@ | |
from astropy.units import Quantity, Unit | ||
from astropy.utils import lazyproperty | ||
from pydantic import ( | ||
AfterValidator, | ||
BaseModel, | ||
BeforeValidator, | ||
ConfigDict, | ||
|
@@ -54,6 +56,32 @@ | |
) | ||
|
||
|
||
# Make a type for a non-empty string for use in name fields | ||
# Names may not have any leading or trailing spaces, and cannot simply | ||
# be spaces. Though this could be implemented as a regular expression | ||
# pattern, that leads to a validation message likely to confuse users, | ||
# along the lines of (for pattern r"^\S$|^\S.*\S$"): | ||
# | ||
# String should match pattern "^\\S$|^\\S.*\\S$" | ||
# | ||
# Instead, the custom validation function below checks for errors and | ||
# raises a ValueError with a more user-friendly message. | ||
def _non_empty_string_validator(value): | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is the intent. I thought about just stripping out whitespace silently but it seemed but to flag it instead. |
||
# Name must have exactly one non-whitespace character or | ||
# at least two non-whitespace characters with any amount of | ||
# other characters between them. | ||
raise ValueError("name must not have leading or trailing whitespace.") | ||
|
||
return value | ||
|
||
|
||
NonEmptyStr = Annotated[str, AfterValidator(_non_empty_string_validator)] | ||
|
||
|
||
def _extract_short_description(docstring: str) -> str: | ||
""" | ||
Extract the first line of the docstring as a short description. | ||
|
@@ -221,9 +249,10 @@ class Camera(BaseModelWithTableRep): | |
model_config = MODEL_DEFAULT_CONFIGURATION | ||
|
||
name: Annotated[ | ||
str, | ||
NonEmptyStr, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took me a bit, but I see this is now using that NonEmptyStr validator you set up above... |
||
Field( | ||
description="Name of the camera", examples=["SBIG ST-8300M", "ZWO ASI1600"] | ||
description="Name of the camera", | ||
examples=["SBIG ST-8300M", "ZWO ASI1600"], | ||
), | ||
] | ||
data_unit: UnitType = Field( | ||
|
@@ -479,7 +508,7 @@ class Observatory(BaseModelWithTableRep): | |
|
||
""" | ||
|
||
name: Annotated[str, Field(description="Name of the observatory")] | ||
name: Annotated[NonEmptyStr, Field(description="Name of the observatory")] | ||
latitude: Annotated[ | ||
Latitude, | ||
_UnitQuantTypePydanticAnnotation, | ||
|
@@ -780,7 +809,11 @@ class PassbandMap(BaseModelWithTableRep): | |
""" | ||
|
||
name: Annotated[ | ||
str, Field(description="Name of the passband map", examples=["Filter wheel 1"]) | ||
NonEmptyStr, | ||
Field( | ||
description="Name of the passband map", | ||
examples=["Filter wheel 1"], | ||
), | ||
] | ||
your_filter_names_to_aavso: list[PassbandMapEntry] | None | ||
|
||
|
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.
I don't think this has to do with name field restrictions. :)
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.
Yeah, we were getting a test failure on two tests because of this