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 scala-xml module #664

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add scala-xml module #664

wants to merge 5 commits into from

Conversation

bplommer
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #664 (89426bb) into main (14c756d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #664      +/-   ##
==========================================
+ Coverage   85.02%   85.03%   +0.01%     
==========================================
  Files         124      125       +1     
  Lines        1629     1631       +2     
  Branches       38       38              
==========================================
+ Hits         1385     1387       +2     
  Misses        244      244              
Impacted Files Coverage Δ
modules/scalaxml/src/main/scala/XmlCodecs.scala 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

build.sbt Outdated
@@ -198,10 +198,23 @@ lazy val circe = crossProject(JVMPlatform, JSPlatform)
)
)

lazy val `scala-xml` = crossProject(JVMPlatform, JSPlatform)
Copy link
Member

Choose a reason for hiding this comment

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

Probably easiest to make it JVM only for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That requires rearranging the tests too, since they're cross-built for js and jvm. I've tried moving the test into jvm-only sources, let's see what happens.

Copy link
Member

Choose a reason for hiding this comment

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

Right, nothing wrong with that. Still, better not to publish for JS unless you have a cross-platform parser. scala-xml.js is kind of a lie at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we'd then need to also exclude the js version from mima? Might be better to reorganize the modules instead to do this properly

Copy link
Member

@armanbilge armanbilge Jun 13, 2022

Choose a reason for hiding this comment

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

I mean, it's pretty simple. Instead of using crossProject, just make an ordinary project :)

The tricky part is for the tests, you need to do a .jvmConfigure(_.dependsOn(`scala-xml`))

@bplommer bplommer marked this pull request as draft June 13, 2022 16:31
@bplommer
Copy link
Contributor Author

Needs mima-wrangling to exclude the new module.

@armanbilge
Copy link
Member

tlVersionIntroduced := List("2.12", "2.13", "3").map(_ -> "skunk version this will be added").toMap

@bplommer
Copy link
Contributor Author

Note on testing - I've copied the pattern of the Circe tests but they aren't really adequate (they'll still pass if you change the Postgres types of the codecs.) I've done some manual end-to-end testing but we don't have any automated tests that these codecs actually work correctly with Postgres.

@bplommer bplommer marked this pull request as ready for review June 14, 2022 09:43
Comment on lines +14 to +15
val xml: Codec[Elem] = Codec.simple(_.toString, s => Either.catchNonFatal(XML.loadString(s)).leftMap(_.getMessage), Type.xml)
val _xml: Codec[Arr[Elem]] = Codec.array(_.toString, s => Either.catchNonFatal(XML.loadString(s)).leftMap(_.getMessage), Type._xml)
Copy link
Member

Choose a reason for hiding this comment

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

If we use fs2-data-xml for parsing then it can cross-compile for JS/Native.

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