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

add opt-in strict string format #451

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

zzstoatzz
Copy link
Contributor

@zzstoatzz zzstoatzz commented Nov 22, 2024

adds pydantic validation per #406 (comment). see the usage example

does not address open questions regarding the language types or re2. happy to chat on those if they'd be blocking here

may close #406

notes

I ran this to generate models

uvx poetry run atproto gen models

and this to update docs

uvx poetry run make -s -C docs gen

and this to run the new tests

uvx poetry run pytest tests/test_atproto_client/models/tests/test_string_formats.py

if desirable, I can follow up this PR to add contribution instructions to the current README section (for uv at least, and perhaps a TODO for proper poetry usage, since I don't really use that) - let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know:

  • if you know a good place to go grab some representative data for these

  • if I should add tests elsewhere covering generated interfaces

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for generated interfaces. but it will be good to have smth like this:

class TestModel(ModelBase):
    did_field: str_frmt.DidField...


def test_did_field_validation:
    test_data = {did_field: blaablabla}
    with pytestExpectException(ValueError) as exc_info:
        get_or_create(test_data, TestModel, string_strings=True)

	assert 'did invalid` in str(exc_info)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

speaking of test data i will try to find it later somewhere in official bsky repo

Copy link
Owner

@MarshalX MarshalX Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzstoatzz hi pls use this files for tests https://github.com/bluesky-social/atproto/tree/main/interop-test-files/syntax. Just download it from the official repo and put to this one. pls do not rename of modify these files. because we want to keep it with easy sync with the official repo without additional effort

Comment on lines +33 to +41
def only_validate_if_strict(validate_fn: core_schema.WithInfoValidatorFunction) -> Callable:
"""Skip validation if not opting into strict validation."""

def wrapper(v: str, info: ValidationInfo) -> str:
if not (info and isinstance(info.context, Mapping) and info.context.get(_OPT_IN_KEY, False)):
return v
return validate_fn(v, info)

return wrapper
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's the critical "pydantic usage" making use of validation context

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we try to invert this if statement? to avoid using of "not"

smth like:

Suggested change
def only_validate_if_strict(validate_fn: core_schema.WithInfoValidatorFunction) -> Callable:
"""Skip validation if not opting into strict validation."""
def wrapper(v: str, info: ValidationInfo) -> str:
if not (info and isinstance(info.context, Mapping) and info.context.get(_OPT_IN_KEY, False)):
return v
return validate_fn(v, info)
return wrapper
def only_validate_if_strict(validate_fn: core_schema.WithInfoValidatorFunction) -> Callable:
"""Skip validation if not opting into strict validation."""
def wrapper(v: str, info: ValidationInfo) -> str:
if info and isinstance(info.context, Mapping) and info.context.get(_OPT_IN_KEY, False):
return validate_fn(v, info)
return v
return wrapper

pyproject.toml Outdated Show resolved Hide resolved
@zzstoatzz zzstoatzz changed the title [wip] add opt-in strict string format add opt-in strict string format Nov 23, 2024
@zzstoatzz zzstoatzz marked this pull request as ready for review November 23, 2024 04:49
Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome hard work! thank you! speaking of your gotten knowledges about how to contribute we need to write CONTRIBUTING.md someday. but this will be separated PR for sure

packages/atproto_codegen/models/generator.py Outdated Show resolved Hide resolved
packages/atproto_codegen/models/generator.py Outdated Show resolved Hide resolved
packages/atproto_codegen/models/generator.py Outdated Show resolved Hide resolved
packages/atproto_codegen/models/generator.py Show resolved Hide resolved
packages/atproto_codegen/models/generator.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for generated interfaces. but it will be good to have smth like this:

class TestModel(ModelBase):
    did_field: str_frmt.DidField...


def test_did_field_validation:
    test_data = {did_field: blaablabla}
    with pytestExpectException(ValueError) as exc_info:
        get_or_create(test_data, TestModel, string_strings=True)

	assert 'did invalid` in str(exc_info)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

speaking of test data i will try to find it later somewhere in official bsky repo

pyproject.toml Outdated Show resolved Hide resolved
@zzstoatzz
Copy link
Contributor Author

thanks for the review @MarshalX! comments should be addressed in 6ad722a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement pydantic validators for string format checks
2 participants