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

experiment/issue-175-encoders-without-json-du #188

Conversation

njlr
Copy link
Contributor

@njlr njlr commented Mar 30, 2024

Change Encoder type to return IEncodable, deferring concrete representation of JSON.

Follows from the discussion here: #175 (comment)

Comment on lines 104 to 107
let inline sbyte (value: sbyte) =
{
new IEncodable with
member this.Encode(helpers) =
helpers.encodeIntegralNumber(uint32 value)
}

let inline byte (value: byte) =
{
new IEncodable with
member this.Encode(helpers) =
helpers.encodeIntegralNumber(uint32 value)
}

let inline int16 (value: int16) =
{
new IEncodable with
member this.Encode(helpers) =
helpers.encodeIntegralNumber(uint32 value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note fo later:

We should probably not cast to uint32 see #187

Comment on lines 132 to 119
let inline int (value: int) =
{
new IEncodable with
member this.Encode(helpers) =
helpers.encodeIntegralNumber(uint32 value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note fo later:

We should probably not cast to uint32 see #187

@MangelMaxime
Copy link
Contributor

Thank you for the PR and showing me that it was indeed possible to defer the concrete representation of the JSON.

@MangelMaxime
Copy link
Contributor

I suspect the CI is failing because of a bug in Fable Python trying to understand where type object 'MutableSequence' has no attribute 'from_' is coming from.

@MangelMaxime
Copy link
Contributor

Well ...

member _.encodeList values = JS.Constructors.Array.from values
member _.encodeSeq values = JS.Constructors.Array.from values

I feel like this place is a good place to start my investigation as seeing JS.Constructors in a Python library feel really suspicious 😅

@MangelMaxime MangelMaxime force-pushed the experiment/issue-175-encoders-without-json-du branch from 6604e92 to afa6ac9 Compare May 4, 2024 14:15
@MangelMaxime MangelMaxime marked this pull request as ready for review May 4, 2024 14:19
@MangelMaxime MangelMaxime merged commit c883b29 into thoth-org:main May 4, 2024
3 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.

None yet

2 participants