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

Var-arg pattern (#59) #179

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Conversation

gosubpl
Copy link
Contributor

@gosubpl gosubpl commented May 31, 2017

Refs #59

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! First pass of review.

@@ -139,6 +139,36 @@
Such members already have a return type of @code{Unit} and sometimes this is unexpected.
Adding an explicit return type makes it more obvious.

@sect(DottyVarArgPattern.toString)
@p
Replaces @code{@} symbols in VarArg patterns with a colon (@code{:}). See @lnk{http://dotty.epfl.ch/docs/reference/changed/vararg-patterns.html}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line here has a syntax error, I think the @ symbol needs to be escaped with @@. To test locally run readme/run.


object VarArg {
List(1, 2, 3, 4) match {
case List(1, 2, xs @ _*) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we minimize this example to

// before
case List(1, 2, xs @ _*)
// after
case List(1, 2, _*)

object VarArg {
List(1, 2, 3, 4) match {
case List(1, 2, xs @ _*) =>
val ys: Seq[Int] = xs
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line and below? We should try to have as small tests as possible.

import scala.meta.tokens.Token
import scalafix.Patch

case object VarArg extends Rewrite {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to DottyVarArgPattern.

@gosubpl
Copy link
Contributor Author

gosubpl commented Jun 2, 2017

Ready for the second pass :)

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you @gosubpl !

I am not sure what caused the CI error, I fetched your fork and I am unable to reproduce locally. I restarted the CI.

// before
case List(1, 2, xs @@ _*)
// after
case List(1, 2, _*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing colon?

@olafurpg
Copy link
Contributor

olafurpg commented Jun 2, 2017

I tracked down the issue to a configuration error in the ci build script. I opened #186 to fix this.

@olafurpg
Copy link
Contributor

olafurpg commented Jun 2, 2017

Also, please run ./scalafmt to fix the build failures.

@olafurpg
Copy link
Contributor

olafurpg commented Jun 2, 2017

Please rebase on top of master and see if that fixes the CI errors.

@gosubpl
Copy link
Contributor Author

gosubpl commented Jun 2, 2017

Many thanks @olafurpg ! Rebased & the doc error fixed (plus a minor sbt build issue for local builds). We'll see if it builds :)

@olafurpg
Copy link
Contributor

olafurpg commented Jun 2, 2017

Cool! Looks like all the tests pass. Only thing missing is to run./scalafmt and we are good to go!

@gosubpl
Copy link
Contributor Author

gosubpl commented Jun 2, 2017

Sorry 'bout the fmt, fixed.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 great work @gosubpl

@olafurpg olafurpg merged commit 1e24af7 into scalacenter:master Jun 2, 2017
@gosubpl
Copy link
Contributor Author

gosubpl commented Jun 2, 2017

@olafurpg many thanks for all your help, and thanks for your patience, this was my first PR for scalafix. I hope to add more soon!

@gosubpl gosubpl deleted the vararg-pattern branch June 2, 2017 13:51
@olafurpg olafurpg mentioned this pull request Jun 3, 2017
@olafurpg olafurpg modified the milestone: v0.4.1 Jun 7, 2017
bjaglin pushed a commit to liancheng/scalafix that referenced this pull request May 23, 2023
* use sbt 1.3+ SemanticdbPlugin to get semanticdb output

* test Scala 2.12 rule against Scala 3 input using sbt-projectmatrix

* Check Scala 2.x input/output with Scala 2.x rule (as before)
* Check Scala 3 input/output with Scala 2.12 rule
* Disable coverage (even after `coverage` command) on input/output
  projects as scoverage is not available in Scala 3 and coverage there
  is not useful there anyway
* Limit strict conflictManager to rules to workaround false positives
  caused by org.scala-lang:scaladoc_3:3.0.0

* Scala 3: removeUnused cannot work as compiler support is missing

* Scala 3: package objects imports do not work well

* Scala 3: groupExplicitlyImportedImplicitsSeparately has no effect
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