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

Make NoAutoTupling fixes the insertion of unit #1452

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

mlachkar
Copy link
Collaborator

@mlachkar mlachkar commented Jul 15, 2021

  • and refactoring tests

I don't know if want to insert () when we have this kind of warnings (that are only reported by -deprecation)

adaptation of an empty argument list by inserting () is deprecated: this is unlikely to be what you want
        signature: Some.apply[A](value: A): Some[A]
  given arguments: <none>
 after adaptation: Some((): Unit)

The test was called NoUnitInsertion. Not clear for me if we want that or not. In the other hand, even if it's not a behavior we want, noAutoTupling insert (), if the user has added -deprecation in his options. It wasn't working for 2.13 since the warning message changed from Adaptation ... to adaptation ... for unit insertions.

  • other point: For this rule, we don't have withConfiguration: should we do the same than in removeUnused ?

@mlachkar mlachkar requested a review from bjaglin July 15, 2021 08:36
@mlachkar mlachkar linked an issue Jul 15, 2021 that may be closed by this pull request
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

👍 Looking at #352, it does look like it's something we want(ed)! Regarding withConfiguration, I guess it would be good to add it to guide usage of the rules as it's getting tricky to set up the right scalacOptions indeed!

@@ -0,0 +1,24 @@
package test.noAutoTupling

class NoUnitInsertion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so if I understand well:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • I didn't know why this test wasn't running. I deleted it by mistake!
  • While grouping the tests together for each rule, I remembered that a small thing was missing to make this rule working. Maybe I will go throw my previous Todo and see what I can fix.

Comment on lines -16 to -18
- disable `-Xfatal-warnings`. Unfortunately, the Scala compiler does not support
finer grained control over the severity level per message kind. See
[scalameta/scalameta#924](https://github.com/scalameta/scalameta/issues/924)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you remove that following scalacenter/sbt-scalafix#196? It's specific to sbt-scalafix, but I guess it's not worth mentioning "just" for the other clients, so I am inclined to say that we should ignore mentions of -Xfatal-warnings considering sbt-scalafix is the main client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes and I forgot about the other clients. I think the other client should mention it (or are already menting it)

@mlachkar mlachkar merged commit 99fd847 into scalacenter:main Jul 16, 2021
@mlachkar mlachkar deleted the noautotupling branch August 12, 2021 13:25
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.

Rule NoAutoTupling should insert () to adapt list of argument
2 participants