Skip to content

Scala 2.12.8 update #660

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

Merged
merged 2 commits into from
Dec 6, 2018
Merged

Scala 2.12.8 update #660

merged 2 commits into from
Dec 6, 2018

Conversation

cquiroz
Copy link
Contributor

@cquiroz cquiroz commented Sep 29, 2018

It would be nice upgrading to 2.12.7 to get better compilation speeds. This PR fixes almost evrything but there is a regression:

scala/bug#11174

@cquiroz cquiroz force-pushed the scala-2.12.7 branch 3 times, most recently from c5f4ecc to 3ef4e89 Compare October 3, 2018 14:15
@tpolecat
Copy link
Member

tpolecat commented Oct 3, 2018

Could we leave the Scala version at 2.12.6 for the client code and upgrade everything else? I talked to Seth and they're not sure what the plan is re: 11174.

@cquiroz
Copy link
Contributor Author

cquiroz commented Oct 3, 2018

Hmm, i didn't think of that. let me try. of course it is a pity since that's the place where I want a faster compiler

@cquiroz
Copy link
Contributor Author

cquiroz commented Oct 4, 2018

Somehow I can’t do this, I set the scala version for client but the compilation error persist

Gotta love sbt

Signed-off-by: Carlos Quiroz <[email protected]>
@cquiroz cquiroz changed the title WIP Scala 2.12.7 update Scala 2.12.8 update Dec 5, 2018
@cquiroz
Copy link
Contributor Author

cquiroz commented Dec 5, 2018

We can proceed with this PR now that scala 2.12.8 has been released.

Copy link
Contributor

@sraaphorst sraaphorst left a comment

Choose a reason for hiding this comment

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

This all looks fine. Were the changes made needed for the upgrade?

className = SeqexecStyles.centeredCell.htmlClass,
cellRenderer = stepExposureRenderer(i.instrument)
)))
.filter(_ => exposureVisible)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to p.showExposure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

.set(none))
} else {
noChange
}

upd.getOrElse(noChange)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why this was changed, if you're going to get noChange anyways. (I didn't look in detail, note.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment. Both codes seem equivalent, and IMO the old one looked better.

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 agreee, the new compiler emits a warning about unused variables in the old code. This seems a bug in the compiler and this is the workaround

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’ll ask about this in the scala channels though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a known issue in scala
scala/bug#11175

@@ -202,7 +202,7 @@ object WebServerLauncher extends StreamApp[IO] with LogInitialization with Seqex
// I have taken this from the examples at:
// https://github.com/gvolpe/advanced-http4s/blob/master/src/main/scala/com/github/gvolpe/fs2/PubSub.scala
// It's not very clear why we need to run this inside a Scheduler
Scheduler[IO](corePoolSize = 4).flatMap { implicit S =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen implicit S like this before... can you explain to me briefly what it means / how it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think I took this from an example while doing the painful scalaz-stream to fs2 port. I don't honestly know 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tpolecat maybe knows?

Copy link
Contributor

@jluhrs jluhrs left a comment

Choose a reason for hiding this comment

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

LGTM

@cquiroz
Copy link
Contributor Author

cquiroz commented Dec 6, 2018

In case you wonder why the update, scala 2.12.7/8 brings a compilation speed improvement of about 10%

@sraaphorst
Copy link
Contributor

sraaphorst commented Dec 6, 2018

@cquiroz Excellent. Thanks for taking the time to answer my questions. I'm still confused, though, by what's happening with the .flatMap{ implicit S =>... anyone explain? Why is implicit being used here? What is implicit?

Signed-off-by: Carlos Quiroz <[email protected]>
@cquiroz
Copy link
Contributor Author

cquiroz commented Dec 6, 2018

Keeping this branch up to date is kind of painful so i’d Rather merge now

@cquiroz cquiroz merged commit 1862f37 into gemini-hlsw:develop Dec 6, 2018
armanbilge pushed a commit to gemini-hlsw/giapi-scala that referenced this pull request Oct 2, 2023
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.

4 participants