Skip to content

Auto-format imports #518

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 1 commit into from
Aug 30, 2021
Merged

Auto-format imports #518

merged 1 commit into from
Aug 30, 2021

Conversation

japgolly
Copy link
Contributor

@japgolly japgolly commented Aug 29, 2021

This is part of #479. I'd love to do all of #479 on the 1.x branch but it would probably just make it harder to merge the outstanding PRs. Doing just the import side now though shouldn't be very disruptive and it lays a little more groundwork.

run: sbt "++${{ matrix.scalaversion }}" package
run: sbt -DCI=1 "++${{ matrix.scalaversion }}" package
Copy link
Member

Choose a reason for hiding this comment

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

What does DCI=1 do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Facilitates this:

scala-js-dom/build.sbt

Lines 31 to 41 in b0fa8f9

val inCI = Option(System.getenv("CI")).exists(_.contains("1"))
val commonSettings = Seq(
organization := "org.scala-js",
scalacOptions ++= Seq(
"-deprecation",
"-feature",
),
scalacOptions ++= (if (!inCI) Seq.empty else Seq(
"-Xfatal-warnings",
)),

So -Xfatal-warnings isn't enabled locally, we need actual warnings so that scalafix can identify and remove unnecessary imports. Where as in CI, we expect that scalafix has already been run (as part of prePR) so we turn those warnings back into errors.

Copy link
Member

Choose a reason for hiding this comment

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

Wait a minute, doesn't -D set system properties and not environment variables?

@japgolly japgolly requested review from armanbilge and removed request for armanbilge August 29, 2021 23:15
@japgolly
Copy link
Contributor Author

Oh I see you already approved, cool, merging

@japgolly japgolly merged commit c410de9 into series/1.x Aug 30, 2021
@japgolly japgolly deleted the topic/formatImports branch August 30, 2021 00:31
@armanbilge
Copy link
Member

@japgolly Oops, maybe I should have revoked my approval until you clarified #518 (comment) 😅

@japgolly
Copy link
Contributor Author

faaaaaaaaaaaark, good catch

japgolly added a commit that referenced this pull request Aug 30, 2021
Follow up to #518
@japgolly japgolly mentioned this pull request Aug 30, 2021
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