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

Enable scalafix on scala 3 #1227

Merged
merged 3 commits into from
Jan 26, 2022
Merged

Conversation

mlachkar
Copy link
Contributor

@mlachkar mlachkar commented Dec 6, 2021

This pull request enable scalafix on Scala3.
I have also organised a little the build, I can remove this from my change.
Thanks!

@mlachkar mlachkar force-pushed the scalafix branch 3 times, most recently from 1b8f913 to fb1d5ca Compare December 7, 2021 09:55
Copy link
Member

@ruippeixotog ruippeixotog left a comment

Choose a reason for hiding this comment

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

I'm sorry for the long turnaround time, @mlachkar! Thanks for creating this pull request 🙂

This LGTM overall, but the build reorganization makes it a bit difficult to understand and I personally prefer using the top-level scope to define ThisBuild settings rather than wrapping them in inThisBuild. Looks like the project changes also caused CI errors.

Do you mind reverting that part and leave only the Scala 3 enabling? Thanks again!

if: startsWith(matrix.scala, '2')
run: sbt "++${{ matrix.scala-version }} scalafixCheckAll"
- name: Check formatting and Scalafix
run: sbt "++${{ matrix.scala-version }}" scalafmtCheckAll scalafmtSbtCheck scalafixCheckAll
Copy link
Member

Choose a reason for hiding this comment

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

The difference in quotes is subtle, but significant. Here we actually want both scalafmtCheckAll and scalafixCheckAll in the same command as the version change - see my explanation about the differences between them in #1188. Can you please change this to either:

  • sbt "++${{ matrix.scala-version }} scalafmtCheckAll scalafixCheckAll" scalafmtSbtCheck
  • sbt "++${{ matrix.scala-version }} scalafmtCheckAll" "++${{ matrix.scala-version }} scalafixCheckAll" scalafmtSbtCheck

(I don't know if the first version works as is, so please test before submitting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one works!

@mlachkar
Copy link
Contributor Author

I kept the linting test and formatting test separated even if the second command runs correctly!
I think it's interesting to be able to figure out quickly if it's a formatting issue or scalafix one, but I can change this PR to do that sbt "++${{ matrix.scala-version }} scalafmtCheckAll" "++${{ matrix.scala-version }} scalafixCheckAll" scalafmtSbtCheck.

@mlachkar mlachkar requested a review from ruippeixotog January 12, 2022 17:34
@mlachkar
Copy link
Contributor Author

By the way, pureconfig is one of my favourite libraries! Thanks for your work!

Copy link
Member

@ruippeixotog ruippeixotog left a comment

Choose a reason for hiding this comment

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

Thanks again!

@ruippeixotog ruippeixotog merged commit 0d53bc7 into pureconfig:master Jan 26, 2022
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