Fix #2837: Remove the term "Scala.js defined" #2997

gzm0 · 2017-06-07T18:24:27Z

It is replaced by non-native JS class/type (except for tests).

With this commit, the following regexes (case insensitive) to not return
any result outside of tests and the definition of the @ScalaJSDefined
annotation itself.

Scala\.js[- ]defined
SJS ?defined
ScalaJSDefined

gzm0 · 2017-06-07T18:25:18Z

Let me know if you want me to:

Fix the test names in this PR (I'd rather not to avoid too many conflicts).
Not close Terminology: Remove Scala.js defined JS classes #2837 with this PR.

scala-jenkins · 2017-06-07T19:52:10Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/4040/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/5351/
Test FAILed.

sjrd

A couple trivial things.

sjrd · 2017-06-07T19:42:25Z

compiler/src/test/scala/org/scalajs/core/compiler/test/JSInteropTest.scala

@@ -203,21 +203,6 @@ class JSInteropTest extends DirectTest with TestHelpers {
      obj <- Seq("class", "trait", "object")
    } yield {
      s"""
-      @ScalaJSDefined


Hum you should only remove the annotation, not the entire test case.

This test case is for a non-native JS class/trait/object, that extends js.Object, and that has an @JSImport annotation with a globalFallback. There's no other test that does that.

sjrd · 2017-06-07T20:53:53Z

compiler/src/main/scala/org/scalajs/core/compiler/GenJSCode.scala

@@ -2247,7 +2247,7 @@ abstract class GenJSCode extends plugins.PluginComponent
          sym.hasFlag(reflect.internal.Flags.DEFAULTPARAM) &&
          isRawJSType(sym.owner.tpe) && {
            /* If this is a default parameter accessor on a
-             * ScalaJSDefinedJSClass, we need to know if the method for which we
+             * NonNativeJSClass, we need to know if the method for which we


"non-native JS class"?

sjrd · 2017-06-07T21:02:21Z

compiler/src/main/scala/org/scalajs/core/compiler/PrepJSInterop.scala

    val JSClass = new OwnerKind(0x10)
-    /** A Scala.js-defined JS oobject. */
+    /** A non-native JS oobject. */


Take the opportunity to fix the oo in oobject.

sjrd · 2017-06-07T21:03:40Z

compiler/src/test/scala/org/scalajs/core/compiler/test/InternalAnnotationsTest.scala

-  def sjsDefinedAnonymousClass: Unit = {
-    test("SJSDefinedAnonymousClass")
+  def anonymousJSClass: Unit = {
+    test("AnonymousJSClass", "scala.scalajs.js.annotation.internal.AnonymousJSClass")
  }


There will be a silent conflict with b3f5839 here. The fully qualified name won't be necessary.

sjrd · 2017-06-07T21:18:52Z

Can you point me to a surviving "Scala.js-defined" in tests? It seems you did rename things in compiler tests and testSuite, so I'm not sure what else there is?

sjrd · 2017-06-07T21:20:42Z

Anyway IMO we can close #2837 with this PR, as there won't be any user-facing references to "Scala.js-defined" left, which is by far the most important.

gzm0 · 2017-06-09T15:53:01Z

Can you point me to a surviving "Scala.js-defined" in tests

For example here:

scala-js/test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/JSSymbolTest.scala

Line 29 in bf0b6f4

@Test def sjsdefined_with_defs_that_are_properties(): Unit = {

There are plenty of these left.

gzm0 · 2017-06-09T15:53:04Z

Also updated.

sjrd · 2017-06-09T16:34:06Z

There are plenty of these left.

Ah yes I see. Let's leave them alone for now, indeed.

scala-jenkins · 2017-06-09T17:44:53Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/4048/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/5361/
Test FAILed.

gzm0 · 2017-06-14T08:12:40Z

Updated (there was a line number issue in a test).

sjrd

Re-LGTM

scala-jenkins · 2017-06-14T12:29:57Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/4062/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/5381/
Test FAILed.

sjrd · 2017-06-14T13:00:27Z

compiler/src/test/scala/org/scalajs/core/compiler/test/JSSAMTest.scala

@@ -49,7 +48,6 @@ class JSSAMTest extends DirectTest with TestHelpers {
      def foo(x: Int): Int
    }

-    @ScalaJSDefined


This change provokes some more line number changes below: at lines 61 and 64, the line numbers should be 15 and 16, respectively.

They probably creeped in from the 0.6.x branch (or forgotten imports).

It is replaced by non-native JS class/type (except for tests). With this commit, the following regexes (case insensitive) to not return any result outside of tests and the definition of the @ScalaJSDefined annotation itself. Scala\.js[- ]defined SJS ?defined ScalaJSDefined

scala-jenkins · 2017-06-14T20:35:17Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/4070/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/5389/
Test PASSed.

gzm0 requested a review from sjrd June 7, 2017 18:24

sjrd requested changes Jun 7, 2017

View reviewed changes

gzm0 force-pushed the no-sjs-defined branch from eeed70c to fde6f29 Compare June 9, 2017 15:51

sjrd approved these changes Jun 9, 2017

View reviewed changes

gzm0 force-pushed the no-sjs-defined branch from fde6f29 to 7edfee8 Compare June 14, 2017 08:12

sjrd approved these changes Jun 14, 2017

View reviewed changes

sjrd reviewed Jun 14, 2017

View reviewed changes

gzm0 added 2 commits June 14, 2017 19:32

Remove unnecessary @ScalaJSDefined

6114a92

They probably creeped in from the 0.6.x branch (or forgotten imports).

gzm0 force-pushed the no-sjs-defined branch from 7edfee8 to 75d81c0 Compare June 14, 2017 17:32

sjrd merged commit ef96a15 into scala-js:master Jun 14, 2017

gzm0 deleted the no-sjs-defined branch June 14, 2017 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2837: Remove the term "Scala.js defined" #2997

Fix #2837: Remove the term "Scala.js defined" #2997

gzm0 commented Jun 7, 2017

gzm0 commented Jun 7, 2017

scala-jenkins commented Jun 7, 2017

sjrd left a comment

sjrd Jun 7, 2017

sjrd Jun 7, 2017

sjrd Jun 7, 2017

sjrd Jun 7, 2017

sjrd commented Jun 7, 2017

sjrd commented Jun 7, 2017 •

edited

Loading

gzm0 commented Jun 9, 2017

gzm0 commented Jun 9, 2017

sjrd commented Jun 9, 2017

scala-jenkins commented Jun 9, 2017

gzm0 commented Jun 14, 2017

sjrd left a comment

scala-jenkins commented Jun 14, 2017

sjrd Jun 14, 2017

scala-jenkins commented Jun 14, 2017

Fix #2837: Remove the term "Scala.js defined" #2997

Fix #2837: Remove the term "Scala.js defined" #2997

Conversation

gzm0 commented Jun 7, 2017

gzm0 commented Jun 7, 2017

scala-jenkins commented Jun 7, 2017

sjrd left a comment

Choose a reason for hiding this comment

sjrd Jun 7, 2017

Choose a reason for hiding this comment

sjrd Jun 7, 2017

Choose a reason for hiding this comment

sjrd Jun 7, 2017

Choose a reason for hiding this comment

sjrd Jun 7, 2017

Choose a reason for hiding this comment

sjrd commented Jun 7, 2017

sjrd commented Jun 7, 2017 • edited Loading

gzm0 commented Jun 9, 2017

gzm0 commented Jun 9, 2017

sjrd commented Jun 9, 2017

scala-jenkins commented Jun 9, 2017

gzm0 commented Jun 14, 2017

sjrd left a comment

Choose a reason for hiding this comment

scala-jenkins commented Jun 14, 2017

sjrd Jun 14, 2017

Choose a reason for hiding this comment

scala-jenkins commented Jun 14, 2017

sjrd commented Jun 7, 2017 •

edited

Loading