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

Make scalaxml module JVM-only #614

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

fthomas
Copy link
Owner

@fthomas fthomas commented Feb 5, 2019

This makes the scalaxml module only available for the JVM because
scala-xml is not available for Scala Native and fails with linking
errors on Scala.js:

Referring to non-existent class javax.xml.parsers.SAXParserFactory$
   called from scala.xml.factory.XMLLoader.parser()javax.xml.parsers.SAXParser
   called from scala.xml.XML$.parser()javax.xml.parsers.SAXParser
   called from scala.xml.factory.XMLLoader.loadString(java.lang.String)scala.xml.Node
   called from scala.xml.XML$.loadString(java.lang.String)scala.xml.Node
   called from eu.timepit.refined.scalaxml.string$Xml$.$$anonfun$xmlValidate$1(java.lang.String)scala.xml.Elem
   called from eu.timepit.refined.scalaxml.string$Xml$.xmlValidate()eu.timepit.refined.api.Validate

See also #610.
/cc @howyp

This makes the scalaxml module only available for the JVM because
scala-xml is not available for Scala Native and fails with linking
errors on Scala.js:
```
Referring to non-existent class javax.xml.parsers.SAXParserFactory$
   called from scala.xml.factory.XMLLoader.parser()javax.xml.parsers.SAXParser
   called from scala.xml.XML$.parser()javax.xml.parsers.SAXParser
   called from scala.xml.factory.XMLLoader.loadString(java.lang.String)scala.xml.Node
   called from scala.xml.XML$.loadString(java.lang.String)scala.xml.Node
   called from eu.timepit.refined.scalaxml.string$Xml$.$$anonfun$xmlValidate$1(java.lang.String)scala.xml.Elem
   called from eu.timepit.refined.scalaxml.string$Xml$.xmlValidate()eu.timepit.refined.api.Validate
```
@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #614 into series/0.10 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           series/0.10     #614   +/-   ##
============================================
  Coverage        93.14%   93.14%           
============================================
  Files               67       67           
  Lines              715      715           
  Branches            10       10           
============================================
  Hits               666      666           
  Misses              49       49
Impacted Files Coverage Δ
...ain/scala/eu/timepit/refined/scalaxml/string.scala 100% <ø> (ø)
.../main/scala/eu/timepit/refined/scalaxml/util.scala 100% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aae866...525f20d. Read the comment docs.

@howyp
Copy link
Contributor

howyp commented Feb 6, 2019

Hmmm - did I mess something up with my PR? Sorry if I did.

But if I'm reading this PR right, does it mean that the scala-xml related code that was in core wouldn't have worked on JS/native?

@fthomas
Copy link
Owner Author

fthomas commented Feb 6, 2019

No, your PR was fine. It preserved what was already in the core module and since the core was available for Scala.js and Scala Native it made sense to also build that module for all three platforms.

But you're right, the scala-xml code never worked on JS and native which was the reason for the tests being JVM-only. The reason why it grew this way, was that the scala-xml code was there before we cross-built for other platforms. With your PR and this PR we're being honest again that the XML stuff is only available on the JVM.

@fthomas fthomas merged commit 69cf94f into series/0.10 Feb 7, 2019
@fthomas fthomas deleted the topic/make-scalaxml-jvm-only branch January 9, 2020 19:56
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