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

Add scala-collection-compact Scalafile rule when bump from 2.12 to 2.13 #2799

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

alifirat
Copy link
Contributor

Address #2770

I'm not sure if my rule is too specific to the scala-compact-library and should be more general to the scala bumping version.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 80.69% // Head: 80.69% // No change to project coverage 👍

Coverage data is based on head (3a83ab1) compared to base (f1b1631).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2799   +/-   ##
=======================================
  Coverage   80.69%   80.69%           
=======================================
  Files         149      149           
  Lines        2865     2865           
  Branches      215      215           
=======================================
  Hits         2312     2312           
  Misses        553      553           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

}
},
{
groupId: "org.scala-lang.modules",
Copy link
Member

Choose a reason for hiding this comment

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

question (blocking): Shouldn't this be run when updating Scala itself (instead of scala-collection-compat)?

This comment follows the conventionalcomments.org standard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my remark: I have the feeling that the rule is too specific to the collection itself while it should be more oriented to the scala version.
In this case, what should be the value of newVersion ? "2.13.0" or we case use a expression like 'newVersion = 2.13. ' to match any update to Scala 2.13 ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe this should be:

  {
    groupId: "org.scala-lang",
    artifactIds: ["scala-library", "scala-compiler", "scala-reflect", "scalap"],
    newVersion: "2.13.0",
    doc: "https://github.com/scala/scala-collection-compat#collection213upgrade",
    rewriteRules: ["dependency:Collection213Upgrade@org.scala-lang.modules:scala-collection-migrations:2.8.1"]
  },

Copy link
Member

Choose a reason for hiding this comment

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

WDYT @exoego?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this makes more sense.

The docs of that rewrite also mention that this Scalafix requires scalacOptions: ["-P:semanticdb:synthetics:on"].

In this case, what should be the value of newVersion ? "2.13.0" or we case use a expression like 'newVersion = 2.13. ' to match any update to Scala 2.13 ?

2.13.0 is fine because Scala Steward will use that the rule when it updates from a version < 2.13.0 to a version >= 2.13.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then, if we all agree I can update the PR with your feedbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fthomas @alejandrohdezma thanks for the feedback, I've rebased and updated the PR.
Tell me if it's good to you.

@alifirat alifirat force-pushed the scalafix-scala-compact21 branch from e62bdfc to 3a83ab1 Compare November 30, 2022 22:34
@exoego exoego merged commit 2a5efcc into scala-steward-org:main Dec 1, 2022
@EwoutH
Copy link
Contributor

EwoutH commented Dec 1, 2022

Thanks a lot for this! Any way that I can test it before the next release is tagged?

@alejandrohdezma
Copy link
Member

If you are using the scala-steward-action you can set the scala-version input to 0.16.1-2-2a5efcc4-SNAPSHOT

@fthomas fthomas linked an issue Dec 1, 2022 that may be closed by this pull request
@fthomas
Copy link
Member

fthomas commented Dec 1, 2022

You don't need to use the latest Scala Steward version to test this. Scalafix migrations are available to all Scala Steward instances as soon as they are merged here (unless that is disabled by the --disable-default-scalafix-migrations command-line option).

@EwoutH
Copy link
Contributor

EwoutH commented Dec 1, 2022

Thank you both!

@exoego exoego added this to the 0.17.0 milestone Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run scala-collection-compat on update to Scala 2.13
5 participants