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

TASK: Unify JSON de/encoding behavior #5093

Closed
wants to merge 2 commits into from
Closed

Conversation

bwaidelich
Copy link
Member

No description provided.

@bwaidelich
Copy link
Member Author

The failing CI is unrelated => #5094

@@ -48,13 +48,13 @@ public static function fromAsset(Asset $asset): self
);
}

public static function fromJson(string $json): self
public static function fromJsonString(string $jsonString): self
Copy link
Member

Choose a reason for hiding this comment

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

ah nice i see you adjusted it to be in sync with the other parts thanks :)
I for one am perfectly fine with toJson() and fromJsonString() ... we discussed this briefly already in #4868 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep. I'm still not very fond of it – not only because it is asynchronous but because JSON is a string by definition. But consistency > correctness in this case I'd say

@bwaidelich bwaidelich requested a review from mhsdesign June 13, 2024 07:34
@bwaidelich
Copy link
Member Author

Mostly resolved with #5196

@bwaidelich bwaidelich closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants