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 xml-support [decoder only] #489

Merged
merged 12 commits into from
Jan 10, 2023
Merged

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Oct 5, 2022

Add a number of construct that allow for the parsing of XML data into Smithy4s-generated datatypes. The heavy-lifting (parsing) is provided by fs2-data. We interop with it by implementing the DocumentBuilder typeclass to create instances of an XmlDocument ADT we control.

NB :

Baccata and others added 2 commits October 5, 2022 18:16
Add a number of construct that allow for the parsing of XML data into
Smithy4s-generated datatypes. The heavy-lifting (parsing) is provided
by fs2-data. We interop with it by implementing the `DocumentBuilder`
typeclass to create instances of an XmlDocument ADT we control.
build.sbt Show resolved Hide resolved
Copy link
Member

@kubukoz kubukoz left a comment

Choose a reason for hiding this comment

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

will take a closer look tomorrow

.compile
.last
.flatMap(_.liftTo[IO](new Throwable("BOOM")))
.map(XmlCursor.fromDocument)
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on adding a decoder.readDocument that hides this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dude the PR is still very much a draft, so keep your guns holstered (I'm merely raising it in case in case I get kicked in the head by a horse, so that the code doesn't die with me), but yeah that's where I was heading towards 😄

Copy link
Member

Choose a reason for hiding this comment

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

my brother in Christ, you asked for the review :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my brother in Christ, you asked for the review :trollface:

Nah I said "if you want to have fun", implying that XML is a lot of fun :trollface:

Copy link
Member

Choose a reason for hiding this comment

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

look at you cheeky bastard 😂

schema.compile(this).emap(refinement.asFunction)

def lazily[A](suspend: Lazy[Schema[A]]): XmlDecoder[A] = new XmlDecoder[A] {
lazy val underlying: XmlDecoder[A] = suspend.map(compile(_)).value
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we can avoid the double lazy (Eval has one built in) and move the .value to inside `read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I had tried to remove the double lazy in the json decoder, and it was actually slightly more performant to keep way it was. I was quite surprised by it. I may be wrong, but I think it's because it's may actually hitting to lazy vals on the hotpath.

Copy link
Member

Choose a reason for hiding this comment

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

interesting

hints: Hints,
alternatives: Vector[SchemaAlt[U, _]],
dispatch: Alt.Dispatcher[Schema, U]
): XmlDecoder[U] = ???
Copy link
Member

Choose a reason for hiding this comment

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

why does CI not complain about these

image

oh I guess it's because you didn't aggregate the xml module in root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even so CI wouldn't complain about them I think, because it's not deadcode.

Copy link
Member

Choose a reason for hiding this comment

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

fair nuff

@Baccata Baccata changed the title [WIP] Add xml-support Add xml-support [decoder only] Oct 6, 2022
@Baccata Baccata marked this pull request as ready for review October 6, 2022 14:24
@Baccata Baccata changed the base branch from main to feature/aws-query-protocol January 10, 2023 14:31
@Baccata Baccata merged commit 4e7836a into feature/aws-query-protocol Jan 10, 2023
@Baccata Baccata deleted the xml-support branch January 10, 2023 15:55
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.

3 participants