Closed LeeTibbert closed 3 years ago
Thanks for the PR @LeeTibbert.
It's very good that we could validate the implementation in Scala Native master through the work you did here so far! Thank you for that.
However, it is a bit early to merge the changes in this repo, for several reasons that you identified, but crucially the fact we can't really depend on a SNAPSHOT version of Scala Native.
For the duplication and changes in tests, I hope that by the next release of Scala Native we'll have JUnit support, which will allow to apply the existing tests without any change.
For the duplication in the source of the JS and Native implementation, my recommendation for this specific case is to just duplicate, without trying to be fancy with rewrites/preprocessing. Preprocessing comes with limitations for links to the sources online, that are stored in the SJSIR and NIR for debugging purposes. Since the duplication is very shallow (there is no logic in those implementation, only Scaladocs and dumb forwarders), plus the fact that the tests will be 100% shared, there is no chance to introduce a fix in one version but not the other.
All that said, I suggest we leave this PR dormant until a true release of SN is out, including with JUnit support.
A lot has changed in the SN world since April. In particular, SN now has support for the JUnit environment.
When SN 0.next is announced, I hope to revisit the top of this PR, probably in a new one.
In my Proof of concept sandbox work to help a number of other projects publish for 0.next soon after it releases, I found the availability of portable-scala-reflect to reduce the amount of fiddling: exactly the intent of portable-scala-reflect in the first place.
@lolgab @sideeffffect Thank you for the suggested edits.
I will dust this off and see what other edits the passage of time requires. I suspect that it needs more than point edits. I do not want to embarrass myself and waste other people's time by submitting an obviously flawed PR.
Yes, the recent release of Scala Native 0.4.0 means that other people may be looking for portable-scala reflect support, so time is an issue.
@LeeTibbert This library can simplify implementations of testing libraries, so it comes in very handy :-)
@lolgab Indeed, I got to this PR in the first place by trying to extend your Zio port ;-) That is, you lead me here!
After 5 minutes on the task, it looks like I should do two new PRs, in rapid succession. One would update these changes (i.e. JUnit is now available in SN). The second would bump the version in this repository.
Today is Inauguration Day in the US, so I might not get much done today but I also do not want to block others. (I know the curses I used when I had to do SN-only code in my private Zio work).
PR #24 is required to get Scala versions supported by Scala Native (and blocks further work here).
I have the required current changes working in my sandbox. I will create a superseding PR after PR #24 is merged.
Another attempt https://github.com/portable-scala/portable-scala-reflect/pull/27
This WIP was good at the time it was created but needs a new PR, due to the passage of time. JUnit tests etc are no longer needed, & other changes.
I have the changes for the new PR. When PR #27 or such sorts out I can create that new PR (or someone else can get thru the gate before me, no problem at my end).
Right now, I am trying to work out the required .travis.yml
changes (which
are still evolving). Step by step, for something that should be simple...
It looks like in the very near future this Repository will need to move off Travis, so I am striving for timeliness of .yml change, rather than perfection.
This was a step on the path towards PR #31. Closing in favor of that PR.
This is a first cut, brute force, obvious port of portablescala.reflect to ScalaNative. I believe it raises some engineering and/or maintenance issues which deserve at least a small amount of discussion.
I appreciate kind suggestions & recommendations. Thank you.
I can not tell if it actually works in Zio because of NIR binary version issues. The latter probably will need to wait for an 0.4.0-M3 release.
NB: This PR changes the sbt-scala-js-crossproject from version 0.6.0 to 1.0.0
Travis CI fails for two reasons. The obvious one is that there is no Scala Native 0.4.0-SNAPSHOT version available. The second is that Travis is looking for the doubly non-extant artifact for scala 2.12. I had thought I had restricted the build for SN to scala 2.11.12. Obviously what I tried did not work. An opportunity to learn more sbt & cross-project fu.
When dust settles, README.md needs to be updated.
Use of 0.4.0-SNAPSHOT is fragile.: 1) Devos using SN support need to know to use an 0.4.0-SNAPSHOT version. They also need to know that any 0.4.0-M2 libraryDependencies used in their project need to be re-built for the 0.4.0-SNAPSHOT used.
2) Perhaps better to wait for 0.4.0-M3 before this is merged?
Duplication of source, candidate for stream editor
The code in native is almost identical to the code in js. The former was copied from the later and some names were changed to protect the innocent. This leads to the possibility of someone correcting a bug on one Platform and forgetting/not_knowing to correct it on the other.
Perhaps others with a greater understanding of scala & sbt can suggest a way to avoid/minimize the duplication.
Normally, I would introduce a build step which ran a stream editor to generate the native code from the js code. That takes some extra fussing around here and may be over-kill.
Duplication of ReflectTest.scala
I moved & copied ReflectTest.scala from the shared directory to the corresponding directories for js & jvm. The allows "sbt> test" and relatives to succeed. Unfortunately, it introduces the duplication I detest.
To the best of my knowledge, there is no Junit for ScalaNative. In other projects I have done the grunt work of manually converting the format used by Scala.js to that used by SN. If needed/recommended, I can do that here. I am hesitant because it introduces another variant. Perhaps that is a lesser evil than untested code. At least, Zio will be using the code to test Object instantiation.
With more work than I have time for now, there may be a way to leave ReflectTest.scala in shared and adjust the path used by the native project to only look in and below native/src/test, skipping shared. A bit of a lie about shared but I do not know how to tell cross-project that something is shared by only two out of three Platforms. A problem for another day.
build SN & JAVA
The build shows the same error for both native & jvm. I took a run at figuring it out and failed. Another problem for another day. This does not seem to prevent publishLocal from finishing.
I am not happy with native having an error, but since it is also in the jvm build it is not the greatest defect.