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

use new lilya #439

Closed
wants to merge 1 commit into from
Closed

use new lilya #439

wants to merge 1 commit into from

Conversation

devkral
Copy link
Contributor

@devkral devkral commented Nov 25, 2024

Checklist

  • The code has 100% test coverage.
  • The documentation was properly created or updated (if applicable) following the correct guidelines and appropriate language.
  • I branched out from the latest main or is a sub-branch.

Summary or description

  • Leverage the new lilya encoder API.
  • Have encoders per app.
  • Use eval_str for newer python versions.

@tarsil
Copy link
Collaborator

tarsil commented Nov 25, 2024

@devkral no need for this. I already applied the new version. You can close this

@tarsil tarsil closed this Nov 25, 2024
@devkral devkral reopened this Nov 25, 2024
- per stack encoders via context vars
- use the new encoders API features of lilya 0.11.0
- fix incompatibilities with lilya 0.11.0
@devkral devkral force-pushed the devkral/enhancement/use_new_lilya branch from d694a92 to 5278988 Compare November 26, 2024 10:12
@@ -1608,7 +1621,15 @@ def extend(self, config: PluggableConfig) -> None:
] = State()
self.async_exit_config = esmerald_settings.async_exit_config

self.encoders = self.load_settings_value("encoders", encoders) or []
self.encoders: list[Union[EncoderProtocol, MoldingProtocol]] = list(
Copy link
Collaborator

Choose a reason for hiding this comment

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

the typing here is not actually needed. Did mypy complain?

@@ -2577,13 +2595,16 @@ def default_settings(
"""
return esmerald_settings

async def globalise_settings(self) -> None:
async def globalize_settings(self) -> None:
Copy link
Collaborator

@tarsil tarsil Nov 26, 2024

Choose a reason for hiding this comment

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

globalise with an S, I wrote in british english. Majority of what you think its an Z and I use S its because of that, so back to globalise, please

"""
Making sure the global settings remain as is
after the request is done.
"""
esmerald_settings.configure(__lazy_settings__._wrapped)

# typo
globalise_settings = globalize_settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

rollback to S and remove this

@@ -2594,7 +2615,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:

scope["state"] = {}
await super().__call__(scope, receive, send)
await self.globalise_settings()
await self.globalize_settings()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I won't be making all comments about this but rollback, please

ENCODER_TYPES as LILYA_ENCODER_TYPES, # noqa
Encoder as LilyaEncoder, # noqa
register_encoder as register_encoder, # noqa
ENCODER_TYPES as ENCODER_TYPES_CTX, # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

LILYA_ENCODER_TYPES was ok, no need to rename, please rollback

def is_type(self, value: Any) -> bool:
return isinstance(value, BaseModel) or is_class_and_subclass(value, BaseModel)
PydanticEncoder = ModelDumpEncoder
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

NO need to use the try here. Msgspec is a hard dependency for now. Rollback this

encoder_types = {encoder.__class__.__name__ for encoder in ENCODER_TYPES}
if encoder.__name__ not in encoder_types:
register_encoder(encoder)
json_encoder = json_encode
Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed everything to use json_encode already, no need to use this. Why? Rollback please

from typing_extensions import Annotated, Doc

from esmerald.encoders import Encoder, json_encoder
from esmerald.encoders import ENCODER_TYPES_CTX, Encoder, json_encoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once LILYA_ENCODER_TYPES is rolledback, this should be changed.

Literally, why change to json_encoder when the base already was updated to use the json_encode? Rollback all of this.

@@ -17,15 +20,48 @@ class ORJSONResponse(BaseJSONResponse):
In the same way the JSONResponse is used, so is the `ORJSONResponse`.
"""

@staticmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this here? The transform should be as it was before. In the Response which is the base class of all of them. This is over complicating. No reason

json_encode(
content,
json_encode_fn=partial(
orjson.dumps, option=orjson.OPT_SERIALIZE_NUMPY | orjson.OPT_OMIT_MICROSECONDS
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ok to override since you want orjson here but the transform should be at the base as it was before.

@@ -19,7 +20,7 @@
from orjson import loads
from pydantic import ValidationError, create_model

from esmerald.encoders import ENCODER_TYPES, Encoder
from esmerald.encoders import ENCODER_TYPES_CTX, Encoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again , rollback to what it was, Literally, please

if (
not origin
and hasattr(encoder, "is_type_structure")
and encoder.is_type_structure(annotation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why all the tests are failing its because you removed the custom encoders of Esmerald. They differ a bit from Lilya, intentionally.

Roll everything back to the way it was before. There is a reason why its using the is_type and not the structure. It was ALL working before.

This was not needed whatsoever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, this will work, despite a bit hacky (subclass check in is_type is a bit unexpected), I think I will undo most

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rollback everything. Esmerald is not Lilya and it operates differently. Not a hack

@devkral
Copy link
Contributor Author

devkral commented Nov 26, 2024

This PR is too invasive. I close it and do it more smart

@devkral devkral closed this Nov 26, 2024
@tarsil tarsil deleted the devkral/enhancement/use_new_lilya branch November 26, 2024 10:50
@tarsil
Copy link
Collaborator

tarsil commented Nov 26, 2024

Don't change things that are already working.

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.

2 participants