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

Replace Headers with Dataclasses #1917

Merged
merged 2 commits into from Jul 7, 2023

Conversation

Tsdevendra1
Copy link
Contributor

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

Currently there are some left over pydantic models in the datastructure.headers namespace. This PR replaces them with dataclasses.

Close Issue(s)

Closes #1386

@Tsdevendra1 Tsdevendra1 requested a review from a team as a code owner July 4, 2023 09:16
Union,
cast,
get_args,
Copy link
Contributor

Choose a reason for hiding this comment

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

use the version from typing_extensions for the helpers because these act differently in older py versions

@@ -272,7 +271,33 @@ def to_header(self, include_header_name: bool = False) -> str:

return (f"{self.HEADER_NAME}: " if include_header_name else "") + self._get_header_value()


def dict(
Copy link
Contributor

Choose a reason for hiding this comment

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

so, i dont see why we will need to replicate the pydantic signature here. Do we actually use exclude_unset, exclude_empty etc? Also, since we actually use this stuff internally - we do not need a generalized use here, but only what we actually use.

@@ -352,14 +384,70 @@ def prevent_storing(cls) -> "CacheControlHeader":

return cls(no_store=True)

@classmethod
def _get_type_annotations(cls) -> Dict[str, Type]:
Copy link
Contributor

Choose a reason for hiding this comment

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

so, i think you are reinventing the wheel here. Please rebase on main and than use the logic inside litestar.typing to handle this kind of stuff. There is a class there called FieldDefinition and it is used throughout the codebase to parse types.

return cls._type_annotations

@classmethod
def _convert_to_type(cls, value: str, expected_type: Type) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as previous comment

def test_etag_documentation_only() -> None:
assert ETag(documentation_only=True).value is None


def test_etag_no_value() -> None:
with pytest.raises(ValidationError):
with pytest.raises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

please raise our ValidationException class

@Goldziher
Copy link
Contributor

@Tsdevendra1 can you update the PR with the changes? I would like to get it merged this weekend if possible

@Goldziher
Copy link
Contributor

well, actually i need this now - so i will merge it.

@all-contributors add @Tsdevendra1 code

@allcontributors
Copy link
Contributor

@Goldziher

I've put up a pull request to add @Tsdevendra1! 🎉

@Goldziher Goldziher merged commit 5d350ac into litestar-org:main Jul 7, 2023
11 of 12 checks passed
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.

Enhancement: Replace Headers with Dataclasses
2 participants