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

[no-master] Enable Scala 2.13.0 in the tools and all the other artifacts. #3702

Merged
merged 4 commits into from
Jun 21, 2019

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Jun 18, 2019

It's [no-master] because

  • The @deprecatedName and scopt commits are irrelevant in master.
  • The other two commits are virtually all-conflicts if I try to cherry-pick them on master. An independent forward-port will be easier to review.

sjrd added 3 commits June 18, 2019 15:21
Discovered by new warnings emitted by the 2.13.0 compiler, yeah!
It is not possible to cross-compile `@deprecatedName`s without
warnings between Scala 2.12- and 2.13+, because:

* In 2.12, they require symbol literals as arguments
* In 2.13, symbol literals are deprecated

Since they were deprecated more than two years ago, and are part of
an API that is not used much anyway (JS envs), I believe it is fair
to drop the source compatibility in this case.
This is the first version that supports Scala 2.13.0 (and also the
latest stable version as of this writing).
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

The last commit message contains a typo: s/unabled/enabled


// filterInPlace replaces retain
def filterInPlace(p: (K, V) => Boolean): Unit = {
// Believe it or not, this is the implementation of `retain` in 2.12.x:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hahahahaha, were you anticipating me complaining :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I was anticipating myself looking back at this code and being appalled at the inefficiency of it.

@sjrd sjrd force-pushed the tools-on-scala-2.13.0 branch from b7805c6 to 6eccbef Compare June 19, 2019 14:02
@sjrd
Copy link
Member Author

sjrd commented Jun 19, 2019

Updated.

Besides addressing the review, I have also added the whitelist and blacklist for the scalaTestSuite, so that the CI can pass. This discovered two bugs filed as #3704 and #3705, which should be fixed separately, as followups.

@sjrd sjrd force-pushed the tools-on-scala-2.13.0 branch from 6eccbef to 868b9a8 Compare June 19, 2019 14:12
@sjrd sjrd mentioned this pull request Jun 20, 2019
11 tasks
…cts.

Partest is not yet enabled in this commit, because a dependency of
`partest` 2.13.0, namely `testkit`, was not published. See
scala/bug#11529 upstream.
@sjrd sjrd force-pushed the tools-on-scala-2.13.0 branch from 868b9a8 to fd2343e Compare June 20, 2019 09:07
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

LGTM modulo the pending big decimal issue (filing a bug for later investigation is sufficient IMO).


# Bugs
scala/collection/convert/MapWrapperTest.scala
# Fails for a BigDecimal range with augmented precision (might be an actual bug)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to file an issue for this at least?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed as #3706.

@sjrd sjrd merged commit ab398e8 into scala-js:0.6.x Jun 21, 2019
@sjrd sjrd deleted the tools-on-scala-2.13.0 branch June 21, 2019 08:53
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