Skip to content
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

Merged
merged 3 commits into from
Apr 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/stellarphot/settings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,14 @@ Deleting just the camera settings would be done like this::
from stellarphot.settings import SavedSettings

saved_settings = SavedSettings()
saved_settings.cameras(confirm=True)
saved_settings.cameras.delete(confirm=True)

Finally, you can delete a single camera from the saved settings like this::

from stellarphot.settings import SavedSettings

saved_settings = SavedSettings()
saved_settings.cameras.delete("My Fancy Camera", confirm=True)
saved_settings.cameras.delete(name="My Fancy Camera", confirm=True)

Reference/API
=============
Expand Down
6 changes: 6 additions & 0 deletions stellarphot/io/tess.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,13 @@ def _retrieve_target_file(self):
result = requests.get(
self.aperture_server + "cgi-bin/gaia_to_aij/upload_request.cgi",
params=params,
timeout=15, # If no response in 15 seconds we won't ever get one...
)
if result.status_code != 200:
Copy link
Contributor

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. :)

Copy link
Contributor Author

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

raise requests.ConnectionError(
f"Failed to retrieve target file: {result.text}"
)

links = re.search(
'href="(.+)"',
result.text.replace("\n", ""),
Expand Down
4 changes: 2 additions & 2 deletions stellarphot/io/tests/test_tess_submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import pytest
from astropy.coordinates import SkyCoord
from requests import ConnectionError
from requests import ConnectionError, ReadTimeout
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Expand Down Expand Up @@ -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:
Expand Down
41 changes: 37 additions & 4 deletions stellarphot/settings/models.py
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

Expand All @@ -9,6 +10,7 @@
from astropy.units import Quantity, Unit
from astropy.utils import lazyproperty
from pydantic import (
AfterValidator,
BaseModel,
BeforeValidator,
ConfigDict,
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -221,9 +249,10 @@ class Camera(BaseModelWithTableRep):
model_config = MODEL_DEFAULT_CONFIGURATION

name: Annotated[
str,
NonEmptyStr,
Copy link
Contributor

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand Down
42 changes: 42 additions & 0 deletions stellarphot/settings/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,48 @@ def test_settings_ui_generation(self, model, settings):
assert all(title_present)


@pytest.mark.parametrize(
"model,settings",
[
[Camera, TEST_CAMERA_VALUES.copy()],
[Observatory, DEFAULT_OBSERVATORY_SETTINGS.copy()],
[PassbandMap, DEFAULT_PASSBAND_MAP.copy()],
],
)
class TestModelsWithName:
"""
Tests that are specific to models that have a name property.
"""

@pytest.mark.parametrize(
"bad_name,error_msg",
[
("", "name must not be empty or contain only whitespace"),
(" ", "name must not be empty or contain only whitespace"),
(" ", "name must not be empty or contain only whitespace"),
(
"name with trailing spaces ",
"name must not have leading or trailing whitespace",
),
(
" name with leading spaces",
"name must not have leading or trailing whitespace",
),
],
)
def test_name_cannot_have_awkward_whitespace(
self, model, settings, bad_name, error_msg
):
settings["name"] = bad_name
with pytest.raises(ValidationError, match=error_msg):
model(**settings)

def test_name_unicode_is_ok(self, model, settings):
# Test that the name field can be unicode
settings["name"] = "π"
assert model(**settings).name == "π"


def test_camera_unitscheck():
# Check that the units are checked properly

Expand Down